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.

Sign up for GitHub

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

Jump to bottom

Fix custom task shell doesn't work without manually passing in "run command" arg/flag #181760

Merged
merged 10 commits into from
Dec 12, 2024
Merged

Fix custom task shell doesn't work without manually passing in "run command" arg/flag #181760

merged 10 commits into from
Dec 12, 2024

Conversation

Legend-Master
Copy link
Contributor

@Legend-Master Legend-Master commented May 7, 2023 •
edited by meganrogge
Loading

Fix #181302
Fix #169821

@Legend-Master
Fix custom task shell doesn't work
3f10caf 
(Without manually passing in "run command" arg/flag)
@Legend-Master
Copy link
Contributor Author

@microsoft-github-policy-service agree

@meganrogge meganrogge added this to the May 2023 milestone May 9, 2023
meganrogge
meganrogge previously approved these changes May 9, 2023
View reviewed changes
@meganrogge meganrogge requested a review from dbaeumer May 9, 2023 13:12
@dbaeumer
Copy link
Member

@meganrogge It is a long time ago that I coded that but I think the rational behind this was that the shell property is only used for unknown shells and there the command argument could be totally different. So now we would always '/d', '/c' which could be totally wrong. This is why as soon as you specify a shell you need to provide the command argument.

@Legend-Master
Copy link
Contributor Author

Legend-Master commented May 10, 2023 •
edited
Loading

So now we would always '/d', '/c' which could be totally wrong. This is why as soon as you specify a shell you need to provide the command argument.

Yes, but we're doing this already...

image

I think we could add commands for known shells, and do nothing for others

And some documentations for this would be nice

@Legend-Master
Copy link
Contributor Author

Also fixes #169821

@Legend-Master Legend-Master requested a review from meganrogge May 23, 2023 15:23
@meganrogge meganrogge removed this from the May 2023 milestone May 26, 2023
@Legend-Master
Copy link
Contributor Author

@meganrogge @dbaeumer It's been 3 weeks by now may I get some response? If you think the changes are not appropriate, could we just talk...

@meganrogge meganrogge added this to the September 2023 milestone Aug 28, 2023
@meganrogge meganrogge removed this from the September 2023 milestone Sep 27, 2023
@Legend-Master Legend-Master requested a review from meganrogge March 7, 2024 03:00
@Legend-Master
Copy link
Contributor Author

Sorry for the super late reply, I didn't notice that I didn't publish my comments, somehow the reply comment gets mixed up with pending review comments

meganrogge
meganrogge approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

This makes sense to me and works based on my testing. Thanks!

@vs-code-engineering vs-code-engineering bot added this to the January 2025 milestone Dec 11, 2024
@meganrogge meganrogge enabled auto-merge (squash) December 11, 2024 18:34
@meganrogge meganrogge merged commit fc3cc5b into microsoft:main Dec 12, 2024
7 checks passed
alexr00 added a commit that referenced this pull request Dec 13, 2024
@alexr00
Revert "Fix custom task shell doesn't work without manually passing i…
b4d8dac 
…n "run command" arg/flag (#181760)"

This reverts commit fc3cc5b.
alexr00 added a commit that referenced this pull request Dec 13, 2024
@alexr00
Revert "Fix custom task shell doesn't work without manually passing i…
fa09409 
…n "run command" arg/flag (#181760)" (#236043)

This reverts commit fc3cc5b.
@alexr00 alexr00 modified the milestones: January 2025, On Deck Dec 13, 2024
@alexr00
Copy link
Member

alexr00 commented Dec 13, 2024

We reverted this because it broke tasks on Mac. I don't see a way to re-open the PR though. @meganrogge FYI.

@Legend-Master
Copy link
Contributor Author

Legend-Master commented Dec 13, 2024 •
edited
Loading

We reverted this because it broke tasks on Mac.

@alexr00 Could you explain a bit more about how it broke tasks on Mac? I don't see any platform specific code that could be causing a breakage from just scanning the code

I don't see a way to re-open the PR though.

I can open another PR to reland this if you will (if we can make it not to break on Mac of course)

@Legend-Master
Copy link
Contributor Author

Legend-Master commented Dec 13, 2024 •
edited
Loading

Ah, I see now, args is not undefined on Mac for the default profile, will open a reland

@Legend-Master Legend-Master mentioned this pull request Dec 13, 2024
Merged
@Legend-Master Legend-Master mentioned this pull request Dec 22, 2024
Open
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jan 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Tyriar Tyriar Tyriar left review comments

@meganrogge meganrogge meganrogge approved these changes

@alexr00 alexr00 alexr00 approved these changes

@dbaeumer dbaeumer Awaiting requested review from dbaeumer

Assignees

@meganrogge meganrogge

Labels
None yet
Projects
None yet
Milestone
On Deck
Development

Successfully merging this pull request may close these issues.

Custom task shell executable doesn't work VS Code is still adding /d /c to task commands
5 participants
@Legend-Master @dbaeumer @alexr00 @Tyriar @meganrogge