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 onDidChangeShell event to the API #160900

Merged
merged 9 commits into from
Sep 14, 2022

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Sep 14, 2022

Fixes #160694

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks babakks changed the title Add change shell event Add onDidChangeShell event to the API Sep 14, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks! Couple of comments and this looks good to go

src/vs/workbench/api/common/extHostTerminalService.ts Outdated Show resolved Hide resolved
Comment on lines 9181 to 9185
/**
* An {@link Event} which fires when the default shell changes.
*/
export const onDidChangeShell: Event<string>;

Copy link
Member

Choose a reason for hiding this comment

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

This needs to move into a vscode.proposed.envShellEvent.d.ts file instead to go through our proposal/finalization process.

@Tyriar Tyriar added this to the September 2022 milestone Sep 14, 2022
@Tyriar Tyriar self-assigned this Sep 14, 2022
babakks and others added 3 commits September 14, 2022 18:17
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
Copy link
Contributor Author

babakks commented Sep 14, 2022

@Tyriar If you're okay, I can rebase the commits to avoid garbage commits on the main branch.

Tyriar
Tyriar previously approved these changes Sep 14, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks @babakks, going to merge this in and will bring it to the next (internal) API sync

@Tyriar
Copy link
Member

Tyriar commented Sep 14, 2022

If you're okay, I can rebase the commits to avoid garbage commits on the main branch.

It's fine as they're all linked to the merge commit, the merger can squash or rebase before merging if they want thought the github UI as well.

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
lramos15
lramos15 previously approved these changes Sep 14, 2022
@babakks
Copy link
Contributor Author

babakks commented Sep 14, 2022

Sorry @Tyriar since you had it reviewed already, but had to improve the style for the event property in fca8afc.

Tyriar
Tyriar previously approved these changes Sep 14, 2022
@Tyriar Tyriar dismissed stale reviews from lramos15 and themself via c461129 September 14, 2022 14:32
@Tyriar Tyriar merged commit a4cce8f into microsoft:main Sep 14, 2022
@babakks babakks deleted the add-change-shell-event branch September 14, 2022 14:56
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vscode.env.shell might return empty string to extension if fetched very soon after startup
3 participants