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

Add --appendCommandLine flag for appending to command #15822

Merged
merged 3 commits into from
Aug 24, 2023

Conversation

hanpuliu-charles
Copy link
Contributor

@hanpuliu-charles hanpuliu-charles commented Aug 11, 2023

Added --appendCommandLine flag that when set, appends the command to the preset command in the profile instead of replacing it.

Previously, there was no good way to launch wt while running a command appended to the set command in the profile. Some uses include profiles that are set to login or start an application.

Additional comments: Looking for a review, and expecting additional changes that needs to be done. For example, I am not really sure on how to include the the option's information in the CallForHelp() screen. Also, would be great if someone could guide me on including tests for this new feature. Thanks!

Closes #5528

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Commandline wt.exe's commandline arguments Product-Terminal The new Windows Terminal. labels Aug 11, 2023
@hanpuliu-charles hanpuliu-charles changed the title Commandlineargs Add -appendCommandLine flag for appending to command Aug 11, 2023
@DHowett
Copy link
Member

DHowett commented Aug 11, 2023

Thanks for doing this! I'm going to mark it up for team discussion, as we want to go over the implications on our command-line API 😄

@DHowett DHowett added the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 11, 2023
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.

This is exactly what I was thinking of, thanks!

(we discussed in team sync today and we're all cool with this)

@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 14, 2023
@zadjii-msft zadjii-msft enabled auto-merge (squash) August 14, 2023 21:03
@zadjii-msft zadjii-msft changed the title Add -appendCommandLine flag for appending to command Add --appendCommandLine flag for appending to command Aug 15, 2023
@zadjii-msft
Copy link
Member

I'm tempted to merge this manually. I think the ARM test CI looks a little confused:

##[error]The agent did not connect within the alloted time of 45 minute(s).

From: https://dev.azure.com/ms/terminal/_build/results?buildId=487090&view=logs&jobId=72ba3e4f-18e2-5e45-1ddf-767922c48e29&j=6fcd723e-77c7-5870-fe2a-56649175917b

I queued a rerun. Dunno if that'll work though.

@zadjii-msft
Copy link
Member

@DHowett any ideas why the ARM tests on this PR keep dying after the 60 minute timeout? There's nothing here that should have changed that. Are the ARM tests just that slow?

@DHowett
Copy link
Member

DHowett commented Aug 15, 2023

@DHowett any ideas why the ARM tests on this PR keep dying after the 60 minute timeout? There's nothing here that should have changed that. Are the ARM tests just that slow?

The ARM tests complete in 5-10 minutes on all other branches, so I'm gonna go with almost certainly they are not that slow! 😄

@DHowett DHowett enabled auto-merge (squash) August 15, 2023 18:35
@DHowett
Copy link
Member

DHowett commented Aug 15, 2023

Thank you so much for doing this!

@DHowett
Copy link
Member

DHowett commented Aug 15, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett DHowett disabled auto-merge August 15, 2023 19:36
@DHowett
Copy link
Member

DHowett commented Aug 15, 2023

Oh, I realize now that none of us read your additional comments! Copied here:

Additional comments: Looking for a review, and expecting additional changes that needs to be done. For example, I am not really sure on how to include the the option's information in the CallForHelp() screen. Also, would be great if someone could guide me on including tests for this new feature. Thanks!

So, fortunately CallForHelp is automatically generated by CLI11. You're in the clear there.

@zadjii-msft can give you a hand on how to author tests for commandline things... but I'll be honest: we are very "light" on tests in that area, and we often find it difficult to run through the entire command line -> launched profile testing scenario.

@zadjii-msft
Copy link
Member

Oh derp I missed that too.

Tests for this are in https://github.com/microsoft/terminal/blob/main/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp. I'd think that something like https://github.com/microsoft/terminal/blob/a7a44901c2b514a37f0a3ec90d38d5647ae4c81f/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp#L450C8-L450C8 looks pretty straightforward how to add the test case to it. But yea, as Dustin mentioned, I'm not sure there's a good E2E test here...

@zadjii-msft
Copy link
Member

Actually, you know what, a test like

void TerminalSettingsTests::TestCommandlineToTitlePromotion()

might actually be able to test the NewTerminalArgs + Profile -> TerminalSettings bits of this, to make sure the commandline gets appended.

@zadjii-msft
Copy link
Member

@hanpuliu-charles You still thinking about trying to write tests for this? If you are that's cool, we can hold off on merging this. Or we can just merge now and add the tests in a follow up. Your call ☺️

@hanpuliu-charles
Copy link
Contributor Author

@hanpuliu-charles You still thinking about trying to write tests for this? If you are that's cool, we can hold off on merging this. Or we can just merge now and add the tests in a follow up. Your call ☺️

Hi! I think you can go ahead and merge for now! Thanks for your help!

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-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. zBugBash-Consider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wt.exe] Add support for appending a commandline to a profile's commandline
4 participants