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

Remove terminal detectors except the one which uses the VSCode API #16023

Open
kimadeline opened this issue Apr 22, 2021 · 8 comments
Open

Remove terminal detectors except the one which uses the VSCode API #16023

kimadeline opened this issue Apr 22, 2021 · 8 comments
Labels
area-terminal bug Issue identified by VS Code Team member as probable bug good first issue needs PR Ready to be worked on

Comments

@kimadeline
Copy link

There's a VS Code API that was added a while ago which we could take advantage of to figure out which shell would be launched by opening a terminal #6050 and the upstream VS Code issue microsoft/vscode#75091.

The recommendation in the vscode-python issue was to drop all of our existing shell detection mechanisms in favour of the VS Code API one, however we still have them:

serviceManager.addSingleton<IShellDetector>(IShellDetector, TerminalNameShellDetector);
serviceManager.addSingleton<IShellDetector>(IShellDetector, SettingsShellDetector);
serviceManager.addSingleton<IShellDetector>(IShellDetector, UserEnvironmentShellDetector);
serviceManager.addSingleton<IShellDetector>(IShellDetector, VSCEnvironmentShellDetector);

➡️ Are they still necessary, all of them?

Second, shells and terminal profiles are going to change (see terminal tabs).

➡️ What does that mean for us?

@kimadeline kimadeline added bug Issue identified by VS Code Team member as probable bug triage-needed Needs assignment to the proper sub-team labels Apr 22, 2021
@karthiknadig karthiknadig added area-terminal needs PR and removed triage-needed Needs assignment to the proper sub-team labels Apr 26, 2021
@karrtikr
Copy link

karrtikr commented May 12, 2021

Are they still necessary, all of them?

I think we only use all those classes as a fallback, in case VSCode API is unable to detect the shell (not sure if that's applicable anymore).

As mentioned here the env.shell API was already implemented in the extension microsoft/vscode#121760 (comment), but it does not seem to be working well with the new defaultProfile setting.

microsoft/vscode#121760

Related: #16175

@kimadeline
Copy link
Author

not sure if that's applicable anymore

Yeah, that's why I think we should revisit that (and why I opened the issue), or at least investigate if they are still needed.

@karrtikr karrtikr changed the title Terminal detection improvements Remove terminal detectors except the one which uses the VSCode API Sep 18, 2021
@druuuu
Copy link

druuuu commented Oct 1, 2021

Hi, can I work on this?

@luabud
Copy link
Member

luabud commented Nov 1, 2021

@druuuu because it's been a month, I'm removing you as an assignee so others can work on this if they want. If you would still like to send a PR for this issue let me know and I can add you back 😊

@karrtikr
Copy link

Except for VSCEnvironmentShellDetector, we can delete all other classes.

@luabud
Copy link
Member

luabud commented Sep 18, 2023

We are reserving this issue for Grace Hopper's Open Source Day, which will take place on September 22nd, 2023. If you're part of the event and would like to submit a contribution to this issue, please let us know, and we'll be happy to assign it to you.

For other community members who are not participating in the event, we appreciate your interest in contributing! We kindly request that you wait until after September 22nd to work on this issue. If no one from the event has been assigned to it by then, we'll gladly assign it to you. Thank you for your understanding and support!

@abbylt
Copy link

abbylt commented Sep 22, 2023

@luabud May I please be assigned to this issue?

@luabud
Copy link
Member

luabud commented Oct 20, 2023

H @abbylt, since it's been one month since OSD I'm removing you as an assignee. You can still feel free to submit a PR to this issue if you'd like, but we're going to leave it open for other contributors too.

@luabud luabud removed the ghc-osd label Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-terminal bug Issue identified by VS Code Team member as probable bug good first issue needs PR Ready to be worked on
Projects
None yet
Development

No branches or pull requests

7 participants