Skip to content

Support LSP based completion provider #243528

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
Merged
merged 60 commits into from
May 21, 2025
Merged

Support LSP based completion provider #243528

merged 60 commits into from
May 21, 2025

Conversation

anthonykim1
Copy link
Contributor

@anthonykim1 anthonykim1 commented Mar 14, 2025 •
edited
Loading

Resolves: #241167
Resolves: microsoft/vscode-python#24776

Note: I left several TODOs for when we eventually make this more generic (not too specific for Python)

@anthonykim1 anthonykim1 added this to the March 2025 milestone Mar 14, 2025
@anthonykim1 anthonykim1 self-assigned this Mar 14, 2025
@anthonykim1 anthonykim1 modified the milestones: March 2025, April 2025 Mar 23, 2025
@anthonykim1
Copy link
Contributor Author

TODO: activate language extension (python extensions, pylance) if installed when user types 'python' in terminal

TODO: Need to figure out way to save successful commands in the virtual file so we get the correct completions in future prompts.

meganrogge
meganrogge reviewed Apr 18, 2025
View reviewed changes
src/vs/platform/terminal/common/capabilities/commandDetectionCapability.ts Outdated
@@ -82,7 +84,8 @@ export class CommandDetectionCapability extends Disposable implements ICommandDe

constructor(
private readonly _terminal: Terminal,
@ILogService private readonly _logService: ILogService
@ILogService private readonly _logService: ILogService,
@ILspTerminalDictionaryService private readonly _lspTerminalDictionaryService: ILspTerminalDictionaryService,
Copy link
Contributor

@meganrogge meganrogge Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we want to inject this service here. Can you describe why this is needed? It would be best to encapsulate all lsp stuff in the provider addon.

meganrogge
meganrogge reviewed Apr 18, 2025
View reviewed changes
meganrogge
meganrogge reviewed Apr 18, 2025
View reviewed changes
meganrogge
meganrogge reviewed Apr 18, 2025
View reviewed changes
meganrogge
meganrogge reviewed May 21, 2025
View reviewed changes
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminal.suggest.contribution.ts Outdated
}

if (
this._ctx.instance.shellType !== GeneralShellType.Python &&
Copy link
Contributor

Choose a reason for hiding this comment

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

could we create consts for all of these strings and keep them in lsp files? python, py, and ms-python...

Copy link
Contributor Author

@anthonykim1 anthonykim1 May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Great idea, just resolved this here: 58e810f

Also I removed check for this._ctx.instance.shellLaunchConfig.executable since it seems like its never Python but always upper shell. this._ctx.shellType would be enough check.

meganrogge
meganrogge reviewed May 21, 2025
View reviewed changes
meganrogge
meganrogge requested changes May 21, 2025
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.

Looks good, getting close!

@anthonykim1 anthonykim1 marked this pull request as draft May 21, 2025 17:50
@anthonykim1 anthonykim1 mentioned this pull request May 21, 2025
Open
@anthonykim1 anthonykim1 marked this pull request as ready for review May 21, 2025 19:24
@anthonykim1 anthonykim1 requested a review from meganrogge May 21, 2025 19:24
@anthonykim1
Copy link
Contributor Author

I created this one file called lspTerminalUtil which basically contains all the const used for terminal-lsp related stuff.
Its only three constant now but I'm sure it would get bigger as we support more lsp providers.

@anthonykim1 anthonykim1 mentioned this pull request May 21, 2025
Open
@anthonykim1 anthonykim1 enabled auto-merge (squash) May 21, 2025 19:36
@anthonykim1 anthonykim1 merged commit bf8e289 into main May 21, 2025
8 checks passed
@anthonykim1 anthonykim1 deleted the anthonykim1/lspTerminalCompletionProvider branch May 21, 2025 19:44
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jul 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@meganrogge meganrogge meganrogge approved these changes

Assignees

@anthonykim1 anthonykim1

Labels
terminal-suggest
Projects
None yet
Milestone
May 2025
Development

Successfully merging this pull request may close these issues.

Terminal suggest: Support LSP-based completion provider Implement Python terminal completion provider
3 participants
@anthonykim1 @meganrogge @lszomoru