[Mike Griese (zadjii-msft)]

Summary of the Pull Request

We need the commandline parser available in TerminalSettingsEditor, for #8812. This PR pulls it out of TerminalApp and into it's own lib, and hooks TSE up to reference that lib. This allows the TSE project to create an instance of the arg parser, so it can validate the commands itself.

I originally did this for projects/5, but I didn't end up needing it.

I did not actually do any validation other than "will it build?"

PR Checklist

• Helps Add startupActions to SUI #8812
• I work here

[Michael Niksa (miniksa)]

Just two minor things.

[Carlos Zamora (carlos-zamora)]

Tried to look over the vcxproj. Still consider me somewhat of a newbie for reviewing those files though so only like 50% confident on that stuff haha.

[Carlos Zamora (carlos-zamora)]

Wait... you might be able to get rid of WinRTUtils no? I don't think you're using the macros.

[Mike Griese (zadjii-msft)]

wait shoot I am. AppCommandlineArgs uses RS_A to get the descriptions for args. That'll inevitably explode when I try doing this.

Dustin L. Howett (@DHowett) do you have any clever ideas for a way around this?

[Mike Griese (zadjii-msft)]

G A H

Yep this will crash immediately. I'll work on some more clever solutions in the morning.

[Mike Griese (zadjii-msft)]

Okay, the Terrible solution I have is to have a .../CommandlineArgs/Resources/en-us/Resources.resw, and include it manually in each of TSE and TerminalApp. That'll get the resources added to the correct libraries, so RS_A will still work, regardless of which dll it's being called from.

Dustin L. Howett (@DHowett) Will that still work for localization?

[Dustin L. Howett (DHowett)]

I think that it will require both key instances to be localized separately? I'm not 100% sure actually. It's by file path actually . . . so maybe it actually DOES work ...

[Mike Griese (zadjii-msft)]

@msftbot make sure Dustin L. Howett (@DHowett) signs off on this

[Deleted user (ghost)]

Hello Mike Griese (@zadjii-msft)!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I'll only merge this pull request if it's approved by Dustin L. Howett (@DHowett)

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[Carlos Zamora (carlos-zamora)]

Approving, because I don't really have anything else to add here. Should we hold off on merging this until after v1.6, to reduce loc burden? I'll let you figure that out haha

[Dustin L. Howett (DHowett)]

We will not merge this for 1.6. 😄

[Don-Vito]

Mike Griese (@zadjii-msft) - if we put it as a dependency for settings, then I can move the validation logic from AppLogic::_TryLoadSettings() into settings.

[Mike Griese (zadjii-msft)]

Interesting. That's certainly an idea. We'd still have to have the resources in both TSM and TerminalApp, (instead of TSE and TerminalApp as the PR currently is), right?

[Don-Vito]

Interesting. That's certainly an idea. We'd still have to have the resources in both TSM and TerminalApp, (instead of TSE and TerminalApp as the PR currently is), right?

I guess so 😿

[Dustin L. Howett (DHowett)]

you'll need to add a recursive resource directory using OCResourceDirectory that points to CommandlineArgs, otherwise we will only ever get the english ones. OCRes is the way that the language-named directories get pulled in. 😄

[Dustin L. Howett (DHowett)]

Notes from review -- won't work with loc

[Deleted user (ghost)]

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

[Chester Liu (skyline75489)]

Cool it, bot. (will it listen to me..)

[Mike Griese (zadjii-msft)]

Yea, it listened ☺️

Spoke with Dustin, I'm gonna try the OCResource thing that we're doing in CascadiaPackage.wapproj. I'll need to put it in the consuming projects (TSE, TA). That might just work?

[github-actions]

check-spelling-bot Report

Unrecognized words, please review:

• AAAAA
• AAAAAAAAAAAAA
• BBBBB
• BBBBBBBB
• Gravell
• It'd
• pws
• qos
• RIS
• sxs
• sys
• We'd
• ZZZZZ

Previously acknowledged words that are now absent

apos aspnet bc boostorg ci conpixels cplinfo crt cw cx dahall df dh dw eb EK ev exeuwp fde fea fmtlib gb gh Gravell's hc hh hk hu isocpp Kd KF KJ KU KX lk mintty Nc NVDA Nx oa OI Oj Oo oq Ou Ov pb pinam pv pw px py qi QJ qo QOL ri Rl rmdir ru smalllogo splashscreen storelogo sx sy sz td tf tl tt uk ul unte vcrt vf vk wx xa xamarin xy Yw yx YZ Zc zd zh ZM zu zy

Some files were were automatically ignored

These sample patterns would exclude them:

^src/host/ft_uia/run\.bat$
^src/host/runft\.bat$
^src/host/runut\.bat$
^src/terminal/adapter/ut_adapter/run\.bat$
^src/terminal/parser/delfuzzpayload\.bat$
^src/terminal/parser/ft_fuzzer/run\.bat$
^src/terminal/parser/ft_fuzzwrapper/run\.bat$
^src/terminal/parser/ut_parser/run\.bat$
^src/tools/integrity/packageuwp/ConsoleUWP\.appxSources$
^src/tools/lnkd/lnkd\.bat$
^src/tools/pixels/pixels\.bat$
^src/tools/texttests/fira\.txt$

You should consider excluding directory paths (e.g. (?:^|/)vendor/), filenames (e.g. (?:^|/)yarn\.lock$), or file extensions (e.g. \.gz$)

You should consider adding them to:

.github/actions/spelling/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/migrie/r/commandline-lib-002 branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/9662bc69102e2e152a062a9060cb63d2742582d3.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
(cat '.github/actions/spelling/excludes.txt' - <<EOF
$should_exclude_patterns
EOF
) |grep .|
sort -f |
uniq > '.github/actions/spelling/excludes.txt.temp' &&
mv '.github/actions/spelling/excludes.txt.temp' '.github/actions/spelling/excludes.txt'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/952174632" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  

should_exclude_patterns=$(perl -e '$/=undef;
$_=<>;
exit unless s{(?:You should consider excluding directory paths|You should consider adding them to).*}{}s;
s{.*These sample patterns would exclude them:}{}s;
s{.*\`\`\`([^`]*)\`\`\`.*}{$1}m;
print' < "$comment_body" | grep . || true)

update_files
rm $comment_body
git add -u

[Mike Griese (zadjii-msft)]

OCResourceDirectory is the escape hatch we use because vcxproj cannot have wildcards in it

so it does a recursive lookup of the directory it's given

if you move commandline args to a new folder, we need to have a full recursive <OCResourceDirectory> that includes that folder

(so, one that points to CommandlineArgs\Resources)

/notes from chat, because I obviously won't be able to ever find them again

[Mike Griese (zadjii-msft)]

You know, I honestly don't think I'm coming back to this any time soon. I'm gonna close this to clean up the queue.