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

Onboard VB intellisense page to Unified Settings #72471

Merged
merged 23 commits into from
Apr 29, 2024

Conversation

Cosifne
Copy link
Member

@Cosifne Cosifne commented Mar 9, 2024

image
We have already done this for C# #71780

This is based on #72429 because we use the new attribute to specify the registration.json file

@Cosifne Cosifne force-pushed the dev/shech/UnifiedSettings4 branch from b12cc1c to c3ef061 Compare March 13, 2024 21:10
@Cosifne Cosifne force-pushed the dev/shech/UnifiedSettings4 branch from 0f52a71 to 6b32646 Compare March 13, 2024 21:15
@Cosifne Cosifne marked this pull request as ready for review March 18, 2024 18:57
@Cosifne Cosifne requested a review from a team as a code owner March 18, 2024 18:57
@Cosifne
Copy link
Member Author

Cosifne commented Mar 18, 2024

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@@ -214,4 +214,22 @@ Use enhanced colors;Editor Color Scheme;Inheritance Margin;Import Directives;</v
<data name="An_empty_Visual_Basic_script_file" xml:space="preserve">
<value>An empty Visual Basic script file.</value>
</data>
<data name="Always_add_new_line_on_enter" xml:space="preserve">
Copy link
Member Author

Choose a reason for hiding this comment

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

Need this because unified settings's enum label can only read from package resources now

Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the corresponding strings from their original location, and reference these from the legacy settings page instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possible, let me check the LOC time line

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all of them, some strings in our exsiting page have _ prefix, like:
image for accessiblity purpose

But the _ will be displayed in unified settings page

Dim snippetValue = Me.OptionStore.GetOption(CompletionOptionsStorage.EnterKeyBehavior, LanguageNames.VisualBasic)
If snippetValue = SnippetsRule.Default Then
Dim enterKeyRule = Me.OptionStore.GetOption(CompletionOptionsStorage.EnterKeyBehavior, LanguageNames.VisualBasic)
If enterKeyRule = EnterKeyRule.Default Then
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 in the previous code
snippetValue is EnterKeyRule but compared to a SnippetsRule.Default
Here change it to the correct type EnterKeyRule
(Complier think it's OK, emm? )
But since they all default to value 0, so no functional change in fact.

@Cosifne
Copy link
Member Author

Cosifne commented Apr 22, 2024

Tag @sharwell to revisit this, I remember we used to think there is no test coverage for the json registration file.
Now it's covered, Could you check this again?

@Cosifne Cosifne enabled auto-merge April 29, 2024 19:08
@Cosifne Cosifne merged commit 1abc045 into dotnet:main Apr 29, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 29, 2024
@Cosifne Cosifne deleted the dev/shech/UnifiedSettings4 branch April 29, 2024 20:46
@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure 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