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

Select interpreter command #3213

Merged
merged 9 commits into from
May 23, 2024
Merged

Select interpreter command #3213

merged 9 commits into from
May 23, 2024

Conversation

seeM
Copy link
Contributor

@seeM seeM commented May 21, 2024

Intent

Address #3118.

Approach

Added the workbench.action.languageRuntime.select command with f1: false. Language packs can implement their own commands in the command palette that call down to this, with a languageId.

I replaced the workbench.action.languageRuntime.start command since this feels like it handles all of the same usecases but with a nicer UX. Happy to revert if I missed something.

I also updated smoke tests to use this.

QA Notes

Smoke tests should pass in CI and locally. Try out the R select interpreter command too.

@seeM seeM requested review from jmcphers and testlabauto May 21, 2024 14:41
@seeM
Copy link
Contributor Author

seeM commented May 21, 2024

Smoke test is failing on CI: https://github.com/posit-dev/positron/actions/runs/9176612586/job/25232287089... Need to look into it.

Base automatically changed from fix-python-select-interpreter-already-selected to main May 21, 2024 18:10
@seeM seeM requested a review from nstrayer May 21, 2024 18:12
@seeM seeM force-pushed the select-interpreter-command branch from abc169f to 958b679 Compare May 21, 2024 18:16
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

LGTM modulo some localization nits.


input.canSelectMany = false;
const languageName = languageService.getLanguageName(languageId);
input.title = `Select ${languageName} Interpreter`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This (and several other strings in this file) needs to be localized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, fixed in 653bdf8.

Copy link
Contributor

@testlabauto testlabauto left a comment

Choose a reason for hiding this comment

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

LGTM! Great job!

@seeM seeM merged commit 779d1af into main May 23, 2024
1 of 2 checks passed
@seeM seeM deleted the select-interpreter-command branch May 23, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants