Fix combining wt args and "wt new-tab" args in implicit context #8315

Don-Vito · 2020-11-18T10:28:05Z

Currently when implicit tab command is specified (i.e., we have
parameters for new-tab, but don't have the explicit subcommand name) we
fallback to parsing the entire arg list as new tab command.

However, if we also have a launch profile (or anything else that might in
the future belong to the upper scope) it is passed as a parameter to the
new tab command, failing the parsing.

The idea behind my solution is to run the parser as a prefix command -
i.e., as long as we succeed to parse [options] / [subcommand] we will
parse them (populating the fields like a launch mode), but once we will
discover something unfamiliar, like profile, we will know that the
prefix is over and will handle the remaining suffix as a new tab
command.

PR Checklist

CLA signed.
Tests added/passed
I've discussed this with core contributors already.

Validation Steps Performed

UT added
Manual run of different option

Closes #7318

…ions before the implicit tab command

zadjii-msft

oh well that was easy 😅 I didn't know about prefix_command, thanks for finding this!

DHowett · 2020-11-18T20:40:13Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

+        // we still have unparsed parameters we need to fail
+        if (remainingParams > 0)
+        {
+            throw CLI::ExtrasError(args);


who catches this? Does it end up down on line 92 as a ParseError?

Yes. It is caught there as a parse error. Implemented this to allow future handling. I can handle it immediately if you prefer.

DHowett · 2020-11-18T20:40:46Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

        }
-    }
-    catch (const CLI::CallForHelp& e)


are we losing support for CallForHelp?

From my understanding the CallForHelp was here only to prevent implicit subcommand parsing (the second catch clause). Now we don't have this catch clause, thus we can handle all exceptions the same way by calling _handleExit, that will still show the relevant UI.

DHowett · 2020-11-18T20:49:36Z

Don't mind me editing your bodies and titles -- I like to maintain imperative mood and some line wrapping rules, but I don't want to force the community to do that 😄

Don-Vito

Don't mind me editing your bodies and titles -- I like to maintain imperative mood and some line wrapping rules, but I don't want to force the community to do that 😄

Sure! Actually, I didn't like the title in the first place - lately I simply copy the title of the issue to the PR... not sure it is a good approach 😊

Don-Vito · 2020-11-18T20:51:36Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

+        // we still have unparsed parameters we need to fail
+        if (remainingParams > 0)
+        {
+            throw CLI::ExtrasError(args);


Yes. It is caught there as a parse error. Implemented this to allow future handling. I can handle it immediately if you prefer.

Don-Vito · 2020-11-18T20:53:43Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

        }
-    }
-    catch (const CLI::CallForHelp& e)


From my understanding the CallForHelp was here only to prevent implicit subcommand parsing (the second catch clause). Now we don't have this catch clause, thus we can handle all exceptions the same way by calling _handleExit, that will still show the relevant UI.

DHowett

With my questions answered, I'll sign off. This is great. 😄

DHowett · 2020-11-18T21:24:40Z

So, with regards to copying the issue being the right approach for a title... I like to follow this guide on "good commit messages". It's obviously a personal style or opinion 😄 but it helps me out.

I use the same rules for PR bodies as commits because we use the PR title as the commit and the PR body as the body.

The basic rules I follow:

Title is 72 characters or less
"Imperative" mood. Pretend that it completes the sentence, "This pull request will ..."
- "This pull request will 7333: it crashes when I minimize it"
- "This pull request will Fix the crash on minimize"
- "This pull request will Implementing support for OSC 300" (bad: title describes what the author is doing, not what the commit is doing)
- "This pull request will Implement support for OSC 300" (flows more nicely)

I like to delete the HTML comments <!-- in our template, as well, and format the body to 80 character hard wrap. That's a personal style.

I should get a request out on the msftbot people to remove HTML comments themselves! Otherwise it just clutters up the commit body when it DOES get merged.

Don-Vito · 2020-11-18T21:33:01Z

So, with regards to copying the issue being the right approach for a title... I like to follow this guide on "good commit messages". It's obviously a personal style or opinion 😄 but it helps me out.

I use the same rules for PR bodies as commits because we use the PR title as the commit and the PR body as the body.

The basic rules I follow:
* Title is 72 characters or less

* "Imperative" mood. Pretend that it completes the sentence, "This pull request will ..."
  
  * "This pull request will **7333: it crashes when I minimize it**"
  * "This pull request will **Fix the crash on minimize**"
  * "This pull request will **Implementing support for OSC 300**" (bad: title describes what the author is doing, not what the commit is doing)
  * "This pull request will **Implement support for OSC 300**" (flows more nicely)
I like to delete the HTML comments <!-- in our template, as well, and format the body to 80 character hard wrap. That's a personal style.

I should get a request out on the msftbot people to remove HTML comments themselves! Otherwise it just clutters up the commit body when it DOES get merged.

I like those guidelines and will try to follow them!

Currently when implicit tab command is specified (i.e., we have parameters for new-tab, but don't have the explicit subcommand name) we fallback to parsing the entire arg list as new tab command. However, if we also have a launch profile (or anything else that might in the future belong to the upper scope) it is passed as a parameter to the new tab command, failing the parsing. The idea behind my solution is to run the parser as a prefix command - i.e., as long as we succeed to parse [options] / [subcommand] we will parse them (populating the fields like a launch mode), but once we will discover something unfamiliar, like profile, we will know that the prefix is over and will handle the remaining suffix as a new tab command. ## Validation Steps Performed * UT added * Manual run of different options Closes #7318 (cherry picked from commit 435e457)

ghost · 2021-01-28T18:01:07Z

🎉Windows Terminal v1.5.10271.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-01-28T18:04:39Z

🎉Windows Terminal Preview v1.6.10272.0 has been released which incorporates this pull request.:tada:

Handy links:

7318: the root parsers should be a prefix-command to retrieve all opt…

ee6d677

…ions before the implicit tab command

ghost added Area-Commandline wt.exe's commandline arguments Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Nov 18, 2020

zadjii-msft approved these changes Nov 18, 2020

View reviewed changes

DHowett reviewed Nov 18, 2020

View reviewed changes

DHowett changed the title ~~7318: Fails to launch maximized/fullscreen and the given profile~~ Fix combining wt args and "wt new-tab" args in implicit context Nov 18, 2020

Don-Vito commented Nov 18, 2020

View reviewed changes

DHowett approved these changes Nov 18, 2020

View reviewed changes

DHowett merged commit 435e457 into microsoft:main Nov 18, 2020

Don-Vito deleted the fet/7318-launchmode-implicit-tab branch December 3, 2020 17:02

DHowett mentioned this pull request Dec 4, 2020

Command line arguments: Mixing options and commands doesn't work #8497

Closed

Don-Vito mentioned this pull request Dec 10, 2020

The command line parsing may erroneously parse the parameters of the subcommand #6781

Closed

ghost mentioned this pull request Jan 28, 2021

Fails to launch maximized/fullscreen and the given profile #7318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix combining wt args and "wt new-tab" args in implicit context #8315

Fix combining wt args and "wt new-tab" args in implicit context #8315

Don-Vito commented Nov 18, 2020 •

edited by DHowett

Loading

zadjii-msft left a comment

DHowett Nov 18, 2020

Don-Vito Nov 18, 2020

DHowett Nov 18, 2020

Don-Vito Nov 18, 2020

DHowett commented Nov 18, 2020

Don-Vito left a comment

Don-Vito Nov 18, 2020

Don-Vito Nov 18, 2020

DHowett left a comment

DHowett commented Nov 18, 2020 •

edited

Loading

Don-Vito commented Nov 18, 2020

ghost commented Jan 28, 2021

ghost commented Jan 28, 2021

Fix combining wt args and "wt new-tab" args in implicit context #8315

Fix combining wt args and "wt new-tab" args in implicit context #8315

Conversation

Don-Vito commented Nov 18, 2020 • edited by DHowett Loading

PR Checklist

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett Nov 18, 2020

Choose a reason for hiding this comment

Don-Vito Nov 18, 2020

Choose a reason for hiding this comment

DHowett Nov 18, 2020

Choose a reason for hiding this comment

Don-Vito Nov 18, 2020

Choose a reason for hiding this comment

DHowett commented Nov 18, 2020

Don-Vito left a comment

Choose a reason for hiding this comment

Don-Vito Nov 18, 2020

Choose a reason for hiding this comment

Don-Vito Nov 18, 2020

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Nov 18, 2020 • edited Loading

Don-Vito commented Nov 18, 2020

ghost commented Jan 28, 2021

ghost commented Jan 28, 2021

Don-Vito commented Nov 18, 2020 •

edited by DHowett

Loading

DHowett commented Nov 18, 2020 •

edited

Loading