Move the commandline args to their own project #8841

zadjii-msft · 2021-01-21T12:16:18Z

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

…ence them too

miniksa

Just two minor things.

src/cascadia/CommandlineArgs/pch.h

src/cascadia/TerminalApp/AppLogic.cpp

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.

src/cascadia/CommandlineArgs/CommandlineArgs.vcxproj

carlos-zamora · 2021-01-21T19:24:50Z

src/cascadia/CommandlineArgs/CommandlineArgs.vcxproj

+    <ProjectReference Include="$(OpenConsoleDir)src\cascadia\WinRTUtils\WinRTUtils.vcxproj">
+      <Project>{CA5CAD1A-039A-4929-BA2A-8BEB2E4106FE}</Project>
+      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
+    </ProjectReference>


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

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?

G A H

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

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?

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/CommandlineArgs/CommandlineArgs.vcxproj

src/cascadia/TerminalApp/pch.h

src/cascadia/TerminalApp/AppLogic.cpp

zadjii-msft · 2021-01-22T13:54:54Z

@msftbot make sure @DHowett signs off on this

ghost · 2021-01-22T13:54:59Z

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

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 · 2021-01-22T18:40:57Z

We will not merge this for 1.6. 😄

Don-Vito · 2021-01-30T19:16:54Z

@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 · 2021-02-01T13:16:19Z

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 · 2021-02-01T20:29:55Z

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 😿

DHowett · 2021-02-05T01:36:01Z

src/cascadia/TerminalSettingsEditor/Microsoft.Terminal.Settings.Editor.vcxproj

+    <!-- 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" />


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

DHowett

Notes from review -- won't work with loc

ghost · 2021-02-12T03:00:07Z

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.

skyline75489 · 2021-02-12T04:02:42Z

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

zadjii-msft · 2021-02-12T11:30:08Z

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 · 2021-10-26T17:53:59Z

@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 [email protected]: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 · 2022-01-10T12:02:54Z

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 · 2023-07-20T20:21:37Z

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.

zadjii-msft added 4 commits January 21, 2021 05:35

Move the commandline args to their own project, so Remoting can refer…

b343baf

…ence them too

some project cleanup

1f3874d

change the namespace

456079a

Add the commandline parser to TSE.

69d1965

zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Jan 21, 2021

fix build breaks

eade437

miniksa previously approved these changes Jan 21, 2021

View reviewed changes

src/cascadia/CommandlineArgs/pch.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved

carlos-zamora requested changes Jan 21, 2021

View reviewed changes

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

hey this builds and launches, so that's something

b801b09

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

zadjii-msft and others added 3 commits January 22, 2021 07:55

shouw

81f5faa

Migrate spelling-0.0.19 changes from main

e4821f2

Migrate spelling-0.0.21 changes from main

4147ec1

carlos-zamora previously approved these changes Jan 22, 2021

View reviewed changes

DHowett reviewed Feb 5, 2021

View reviewed changes

DHowett requested changes Feb 5, 2021

View reviewed changes

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 added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Feb 12, 2021

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 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 removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 12, 2021

zadjii-msft self-assigned this Mar 3, 2021

zadjii-msft added this to the Terminal v1.9 milestone Apr 2, 2021

zadjii-msft modified the milestones: Terminal v1.9, Terminal v2.0 May 17, 2021

zadjii-msft mentioned this pull request Aug 26, 2021

Persist window layout on window close #10972

Merged

zadjii-msft modified the milestones: Terminal v2.0, Terminal v1.12 Aug 26, 2021

zadjii-msft mentioned this pull request Sep 1, 2021

megathread: Window State Persistence #9800

Open

16 tasks

zadjii-msft modified the milestones: Terminal v1.12, Engineering Improvements Sep 22, 2021

ghost dismissed stale reviews from carlos-zamora and miniksa via e4821f2 November 2, 2021 13:46

zadjii-msft modified the milestones: Engineering Improvements 2021, Engineering Improvements Jan 10, 2022

zadjii-msft closed this Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the commandline args to their own project #8841

Move the commandline args to their own project #8841

zadjii-msft commented Jan 21, 2021

miniksa left a comment

carlos-zamora left a comment

carlos-zamora Jan 21, 2021

zadjii-msft Jan 21, 2021

zadjii-msft Jan 21, 2021

zadjii-msft Jan 22, 2021

DHowett Jan 22, 2021

zadjii-msft commented Jan 22, 2021

ghost commented Jan 22, 2021

carlos-zamora left a comment

DHowett commented Jan 22, 2021

Don-Vito commented Jan 30, 2021

zadjii-msft commented Feb 1, 2021

Don-Vito commented Feb 1, 2021

DHowett Feb 5, 2021

DHowett left a comment

ghost commented Feb 12, 2021

skyline75489 commented Feb 12, 2021

zadjii-msft commented Feb 12, 2021

github-actions bot commented Oct 26, 2021 •

edited

Loading

zadjii-msft commented Jan 10, 2022

zadjii-msft commented Jul 20, 2023

Move the commandline args to their own project #8841

Move the commandline args to their own project #8841

Conversation

zadjii-msft commented Jan 21, 2021

Summary of the Pull Request

PR Checklist

miniksa left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Jan 21, 2021

Choose a reason for hiding this comment

zadjii-msft Jan 21, 2021

Choose a reason for hiding this comment

zadjii-msft Jan 21, 2021

Choose a reason for hiding this comment

zadjii-msft Jan 22, 2021

Choose a reason for hiding this comment

DHowett Jan 22, 2021

Choose a reason for hiding this comment

zadjii-msft commented Jan 22, 2021

ghost commented Jan 22, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

DHowett commented Jan 22, 2021

Don-Vito commented Jan 30, 2021

zadjii-msft commented Feb 1, 2021

Don-Vito commented Feb 1, 2021

DHowett Feb 5, 2021

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Feb 12, 2021

skyline75489 commented Feb 12, 2021

zadjii-msft commented Feb 12, 2021

github-actions bot commented Oct 26, 2021 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

zadjii-msft commented Jan 10, 2022

zadjii-msft commented Jul 20, 2023

github-actions bot commented Oct 26, 2021 •

edited

Loading