Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the commandline args to their own project #8841

Closed
wants to merge 9 commits into from

Conversation

zadjii-msft
Copy link
Member

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

@zadjii-msft zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Jan 21, 2021
miniksa
miniksa previously approved these changes Jan 21, 2021
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two minor things.

src/cascadia/CommandlineArgs/pch.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/cascadia/CommandlineArgs/CommandlineArgs.vcxproj Outdated Show resolved Hide resolved
Comment on lines +54 to +57
<ProjectReference Include="$(OpenConsoleDir)src\cascadia\WinRTUtils\WinRTUtils.vcxproj">
<Project>{CA5CAD1A-039A-4929-BA2A-8BEB2E4106FE}</Project>
<ReferenceOutputAssembly>false</ReferenceOutputAssembly>
</ProjectReference>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DHowett do you have any clever ideas for a way around this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

G A H

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DHowett Will that still work for localization?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

src/cascadia/TerminalApp/pch.h Show resolved Hide resolved
src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jan 21, 2021
@zadjii-msft
Copy link
Member Author

@msftbot make sure @DHowett signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 22, 2021
@ghost
Copy link

ghost commented Jan 22, 2021

Hello @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 @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 previously approved these changes Jan 22, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DHowett
Copy link
Member

DHowett commented Jan 22, 2021

We will not merge this for 1.6. 😄

@Don-Vito
Copy link
Contributor

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

@zadjii-msft
Copy link
Member Author

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
Copy link
Contributor

Don-Vito commented Feb 1, 2021

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 😿

<!-- Make sure to include the CommandlineArgs resources here, so they show up in our scope -->
<PRIResource Include="$(OpenConsoleDir)src\cascadia\CommandlineArgs\Resources\en-US\Resources.resw">
<SubType>Designer</SubType>
</PRIResource>
<OCResourceDirectory Include="Resources" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😄

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes from review -- won't work with loc

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 5, 2021
@ghost
Copy link

ghost commented Feb 12, 2021

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.

@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Feb 12, 2021
@skyline75489
Copy link
Collaborator

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

@ghost ghost removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Feb 12, 2021
@zadjii-msft zadjii-msft removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 12, 2021
@zadjii-msft zadjii-msft removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 12, 2021
@zadjii-msft
Copy link
Member Author

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
Copy link

github-actions bot commented Oct 26, 2021

@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

@zadjii-msft
Copy link
Member Author

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

@zadjii-msft
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants