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 option for LSP-based auto insert #75224

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

akhera99
Copy link
Member

No description provided.

@akhera99 akhera99 requested a review from a team as a code owner September 24, 2024 20:41
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 24, 2024
@@ -32,6 +32,8 @@ internal sealed class LspOptionsStorage

private static readonly OptionGroup s_codeLensOptionGroup = new(name: "code_lens", description: "");

private static readonly OptionGroup s_onAutoInsertOptionGroup = new(name: "on_auto_insert", description: "");
Copy link
Member

Choose a reason for hiding this comment

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

on_auto_insert doesn't sound like a good option group? Is this related to typing?

Copy link
Member

Choose a reason for hiding this comment

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

Also note I believe the grouping name here would be reflected by vscode in the settings page.
Like inlay hints
image
So a better group name would be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

"On Auto Insert" is the name of the feature, but it's not option-based in roslyn so the naming has never been an issue. We could change it to Auto Insert, @dibarbet @JoeRobich any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

VSCode settings are laid out according to the grouping in the extensions's package.json (Inlay Hints options are under Text Editor). Not really sure what function the OptionGroup plays here.

Copy link
Member

Choose a reason for hiding this comment

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

I do like "Auto Insert" better.

Copy link
Member

Choose a reason for hiding this comment

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

Auto insert also seems fine to me. Option group is used as the item after the first arrow. For example for "Dotnet > Inlay Hints: Suppress...", the option group is "Inlay Hints". I think having that be "Auto Insert" would be good.

Copy link
Member

Choose a reason for hiding this comment

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

VSCode settings are laid out according to the grouping in the extensions's package.json (Inlay Hints options are under Text Editor). Not really sure what function the OptionGroup plays here.

Yeah it's because the option group name is used in package.json

Auto Insert LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants