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 test to make sure option names won't change #69319

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

Cosifne
Copy link
Member

@Cosifne Cosifne commented Aug 1, 2023

Once an option is added to the LSP server, its name and options group should not be modified.
I talked to @333fred during lunchtime and he said it doesn't meet the public API bar so these don't need to go via the API review.

Add a test to guard this

@Cosifne Cosifne requested a review from a team as a code owner August 1, 2023 18:17
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 1, 2023
@@ -26,7 +26,6 @@ internal partial class DidChangeConfigurationNotificationHandler
ImplementTypeOptionsStorage.PropertyGenerationBehavior,
// Completion
CompletionOptionsStorage.ShowNameSuggestions,
CompletionOptionsStorage.ShowItemsFromUnimportedNamespaces,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a duplicate (See line 31)

// These options are persist in the LSP client. Please make sure also modify the LSP client code if these strings are changed.
var expectedNames = new[]
{
"symbol_search.dotnet_search_reference_assemblies",
Copy link
Member

Choose a reason for hiding this comment

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

Should we have this sorted and use one of our sort-ignoring Assert functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's possible

"implement_type.dotnet_insertion_behavior",
"implement_type.dotnet_property_generation_behavior",
"completion.dotnet_show_name_completion_suggestions",
"completion.dotnet_provide_regex_completions",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be something like dotnet.completion.provideRegexCompletions? Or am I misunderstanding what this test is trying to do?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the test here is just to make sure the option name + option group doesn't get changed. (because these strings would be used in the client, if it only gets changed in server client-side storage would be broken)

For completion.dotnet_provide_regex_completions is created from option group + . + option name.
Client would parse the string, and reorder it to dotnet.completion.provideRegexCompletions

@Cosifne Cosifne merged commit b3dc95e into dotnet:main Aug 1, 2023
24 checks passed
@Cosifne Cosifne deleted the dev/shech/forzenOptionNames branch August 1, 2023 23:53
@ghost ghost added this to the Next milestone Aug 1, 2023
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants