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

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

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Nov 18, 2020

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

@ghost 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
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
catch (const CLI::CallForHelp& e)
Copy link
Member

Choose a reason for hiding this comment

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

are we losing support for CallForHelp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 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
@DHowett
Copy link
Member

DHowett commented Nov 18, 2020

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 😄

Copy link
Contributor Author

@Don-Vito Don-Vito left a comment

Choose a reason for hiding this comment

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

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 😊

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

Choose a reason for hiding this comment

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

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

}
}
catch (const CLI::CallForHelp& e)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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.

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

@DHowett DHowett merged commit 435e457 into microsoft:main Nov 18, 2020
@DHowett
Copy link
Member

DHowett commented Nov 18, 2020

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

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!

@Don-Vito Don-Vito deleted the fet/7318-launchmode-implicit-tab branch December 3, 2020 17:02
DHowett pushed a commit that referenced this pull request Jan 25, 2021
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
Copy link

ghost commented Jan 28, 2021

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

Handy links:

@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to launch maximized/fullscreen and the given profile
3 participants