Implement Naming Styles for editorconfig UI by jmarolf · Pull Request #58075 · dotnet/roslyn

jmarolf · 2021-12-02T16:31:38Z

Related to #40163

This PR implements naming style support in 3 steps (and I would recommend reviewing this PR step-by-step)

d77a8f8 Implement a parser for editorconfig files
5e5cbc8 build support on top of the parser for discovering and updating naming style options
a773abe provide a UI on top of these new services

This adds all the capabilities necessary to discover and update naming styles in an editorconfig. depends on the parser functionality added in previous commit

This adds a new tab for the naming style options that were exposed via services in a previous commit

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/readme.md

...ensions/Compiler/Core/NamingStyles/EditorConfig/EditorConfigNamingStyleParser_NamingStyle.cs

src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingStyles/NamingStyleSettingsUpdater.cs

JoeRobich · 2021-12-02T17:54:20Z

src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingStyles/SourceTextExtensions.cs

+                else
+                {
+                    var span = new TextSpan(sourceText.Length, 0);
+                    newNamingStyleSection.Append("\r\n");


Should we use Environment.NewLine here?

src/EditorFeatures/Core/EditorConfigSettings/readme.md

…ngStyles/EditorConfig/EditorConfigNamingStyleParser_NamingStyle.cs Co-authored-by: Joey Robichaud <jorobich@microsoft.com>

…orConfig/readme.md Co-authored-by: Joey Robichaud <jorobich@microsoft.com>

Youssef1313

There is already a parser in the compiler. I'm not a big fan of duplicating part of that logic. Is there a way to reuse the existing compiler support for EditorConfig?

ryzngard · 2021-12-02T18:21:38Z

Implement a parser for editorconfig files

This scares me 👀

Youssef1313 · 2021-12-02T18:38:00Z

One thing I've always wondered about the EditorConfig UI, how will it be able to handle scenarios where different directories have different settings. This is far from being something to do in this PR, but sharing my thought anyway to consider.

jmarolf · 2021-12-02T18:48:28Z

There is already a parser in the compiler. I'm not a big fan of duplicating part of that logic. Is there a way to reuse the existing compiler support for EditorConfig?

I mention this here: https://github.com/jmarolf/roslyn/blob/features/editorconfig-naming-style-support/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/readme.md but then I asked you to review specific commits so this is what I get ¯_(ツ)_/¯

The compilers implementation has some rather big perf requirements and I don't want them to take on the additional complexity . Also the way I've done section parsing includes several cases that the compiler does not need to worry about. Would like to unify these someday but I think the requirements diverge enough that I went with a separate implementation that meets our needs

Youssef1313 · 2021-12-02T18:53:10Z

...spaces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/Parsing/EditorConfigParser.cs

+        // Matches EditorConfig section header such as "[*.{js,py}]", see https://editorconfig.org for details
+        private static readonly Regex s_sectionMatcher = new(@"^\s*\[(([^#;]|\\#|\\;)+)\]\s*([#;].*)?$", RegexOptions.Compiled);
+        // Matches EditorConfig property such as "indent_style = space", see https://editorconfig.org for details
+        private static readonly Regex s_propertyMatcher = new(@"^\s*([\w\.\-_]+)\s*[=:]\s*(.*?)\s*([#;].*)?$", RegexOptions.Compiled);


@jmarolf One of the reasons I'm worried about two parser implementations is something like #51625, which proposes a change to the s_propertyMatcher regex. It's likely we can miss any changes of those kind here. Probably some code comments in each place referring to the other would do it for now until this is unified?

in a world where there was a formal spec that we could write against this would be less of a concern but things being as they are I agree we don't want to do this for no reason. I am still of the opinion that this is justified considering the compilers' requirements.

I will add test cases for the comment parsing to both implementations so that if someone ever updates one version and not the other CI will fail.

...eatures/Core/EditorConfigSettings/Updater/NamingStyles/EditorConfigNamingStylesExtensions.cs

…les/EditorConfigNamingStylesExtensions.cs Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingStyles/SourceTextExtensions.cs

jmarolf · 2021-12-02T19:14:30Z

One thing I've always wondered about the EditorConfig UI, how will it be able to handle scenarios where different directories have different settings. This is far from being something to do in this PR, but sharing my thought anyway to consider.

I would love to hear ideas. My plan is to is to augment the location column (which you added :)) so that you can navigate to different editorconfig or globalconfig files if the one you are in is not where the options definition currently resided.

My expectation is that the UI is a view over a single file showing the rules that are defined there as well as the "implicit" rules that it is pulling from elsewhere. Editing therefore always applies to the file you are viewing but we can make navigating across files better. We can also add additional columns so that sorting and filtering is easier.

Another case that I would like to handle is different rules across different sections in the same file. I currently punt on things like this:

[*.cs]
# C# specific settings
[*.vb]
# Visual Basic specific settings
[*.{cs,vb}]
# Settings for both

If there are duplicated rules. My hope is that I could show duplicated rules as duplicates in the list with a column showing the section they come from to disambiguate

ryzngard

Review 1/3 done

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/LanguageExtensions.cs

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/Parsing/EditorConfigFile.cs

...spaces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/Parsing/EditorConfigOption.cs

...aces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/Parsing/EditorConfigOption`1.cs

...ces/SharedUtilitiesAndExtensions/Compiler/Core/EditorConfig/Parsing/Sections/SectionMatch.cs

...edUtilitiesAndExtensions/Compiler/Core/EditorConfig/Parsing/Sections/SectionMatcher.Lexer.cs

ryzngard

2/3 done

...eatures/Core/EditorConfigSettings/Updater/NamingStyles/EditorConfigNamingStylesExtensions.cs

ryzngard · 2021-12-06T22:46:53Z

src/EditorFeatures/Core/EditorConfigSettings/Data/NamingStyleSetting.cs

+        internal SymbolSpecification? Type { get; set; }
+
+        public string StyleName => Style.Name;
+        public string[] AllStyles => _allStyles.Select(style => style.Name).ToArray();


This should be ImmutableArray<string>

ryzngard · 2021-12-06T22:47:37Z

src/EditorFeatures/Core/EditorConfigSettings/Data/NamingStyleSetting.cs

+
+        internal void ChangeStyle(int selectedIndex)
+        {
+            if (selectedIndex > -1 && selectedIndex < _allStyles.Length && Location is not null)


Should this throw index out of bounds?

src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingStyles/NamingStyleSettingsUpdater.cs

ryzngard · 2021-12-06T22:53:47Z

...aredUtilitiesAndExtensions/Compiler/Core/Utilities/EditorConfigFileGenerator_NamingStyles.cs

+        public static void AppendNamingStylePreferencesToEditorConfig(IEnumerable<NamingRule> namingRules, StringBuilder editorconfig, string? language = null)
+        {
+            var symbolSpecifications = namingRules.Select(x => x.SymbolSpecification).ToImmutableArray();
+            var namingStyles = namingRules.Select(x => x.NamingStyle).ToImmutableArray();


It's probably not a huge deal, but doing ToImmutableArray() here is slightly more expensive than just leaving as an IEnumerable since you're going to just enumerate again and not use the ImmutableArray. That ore combine with the Select below for one chained LINQ

ryzngard

Overall looks good. Nothing blocking, minor feedback here and there. The Readme.md files were really helpful and I'm glad they're being checked in. One slight improvement would be to call them out in the PR description (I know it's tough/impossible to link) just so we know to look for them :) Hopefully we can start adopting this for future PRs as a team

ryzngard · 2021-12-06T22:57:00Z

src/EditorFeatures/Core/EditorConfigSettings/readme.md

@@ -0,0 +1,22 @@
+# Editorconfig settings providers and updaters


👍 this is a great idea. Love the contextual markdown files.

...Def/EditorConfigSettings/NamingStyle/ViewModel/ColumnViewModels/NotifyPropertyChangedBase.cs

jmarolf added 6 commits December 1, 2021 15:21

move Language enum further down the stack

883a2c5

implement naming styles option parser with text span support

d77a8f8

add folder description

a7e051a

Add namging style option support

5e5cbc8

This adds all the capabilities necessary to discover and update naming styles in an editorconfig. depends on the parser functionality added in previous commit

add folder description

682a9df

Add views for naming styles

a773abe

This adds a new tab for the naming style options that were exposed via services in a previous commit

jmarolf requested review from a team and ryzngard December 2, 2021 16:31

ghost added Needs UX Triage Area-IDE labels Dec 2, 2021

JoeRobich reviewed Dec 2, 2021

View reviewed changes

jmarolf and others added 2 commits December 2, 2021 10:01

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Nami…

48f54b4

…ngStyles/EditorConfig/EditorConfigNamingStyleParser_NamingStyle.cs Co-authored-by: Joey Robichaud <jorobich@microsoft.com>

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Edit…

1886364

…orConfig/readme.md Co-authored-by: Joey Robichaud <jorobich@microsoft.com>

Youssef1313 reviewed Dec 2, 2021

View reviewed changes

...eatures/Core/EditorConfigSettings/Updater/NamingStyles/EditorConfigNamingStylesExtensions.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Dec 2, 2021

View reviewed changes

...eatures/Core/EditorConfigSettings/Updater/NamingStyles/EditorConfigNamingStylesExtensions.cs Outdated Show resolved Hide resolved

jmarolf and others added 2 commits December 2, 2021 11:00

Update src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingSty…

49088b0

…les/EditorConfigNamingStylesExtensions.cs Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

Update src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingSty…

da90d81

…les/EditorConfigNamingStylesExtensions.cs Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

Youssef1313 reviewed Dec 2, 2021

View reviewed changes

src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingStyles/SourceTextExtensions.cs Outdated Show resolved Hide resolved

jmarolf mentioned this pull request Dec 2, 2021

Unify SettingsUpdateHelper and ConfigurationUpdater implementation #52735

Closed

Youssef1313 reviewed Dec 2, 2021

View reviewed changes

src/EditorFeatures/Core/EditorConfigSettings/Updater/NamingStyles/SourceTextExtensions.cs Outdated Show resolved Hide resolved

jmarolf added 4 commits December 2, 2021 11:35

lengthen the search box to align with the tabs above it

3a28801

enable nullable on test files

cbce098

de-duplicate local functions

525cb16

simplify boolean logic

03da9e5

jmarolf added 2 commits December 2, 2021 14:15

use resource strings in tests

98006e6

return immutable array for static severities

b53f7d1

This was referenced Dec 3, 2021

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

Test AsyncIteratorWithAwaitAndYieldAndAwait is flaky #57797

Closed

jmarolf force-pushed the features/editorconfig-naming-style-support branch from d1bf3a2 to 4121f55 Compare December 3, 2021 18:06

This was referenced Dec 3, 2021

Transient failure in test AsyncIteratorWithCustomCode #34207

Closed

Failure in CSharpCodeActions.GFUFuzzyMatchAfterRenameTrackingAndAfterGenerateType #57423

Closed

update readme with a relative path link

74e9ca0

jmarolf force-pushed the features/editorconfig-naming-style-support branch from 4121f55 to 74e9ca0 Compare December 3, 2021 22:42

jmarolf added 3 commits December 3, 2021 15:40

using TryPeekNext

761eb1e

using disposeable pooled objects

ba7bdb3

renamge rangeOpt to numberRange

2c9fed1

ryzngard reviewed Dec 4, 2021

View reviewed changes

jmarolf added 4 commits December 6, 2021 11:12

fix Span and Section being swapped from the base

264eefb

update incorrect comment

b2ca734

Combine Lex with static function

a321a3f

return false if we fail to determine the language for a path

132f6b0

runfoapp bot mentioned this pull request Dec 6, 2021

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

use Lazy instead of fancy InterlockedInitialize

cb5cfc0

ryzngard reviewed Dec 6, 2021

View reviewed changes

ryzngard approved these changes Dec 6, 2021

View reviewed changes

move search bar up by 12 pixels

86ef543

vatsalyaagrawal added the Approved to merge label Dec 7, 2021

jmarolf merged commit 032a9be into dotnet:release/dev17.1 Dec 7, 2021

jmarolf deleted the features/editorconfig-naming-style-support branch December 7, 2021 18:51

jmarolf mentioned this pull request Dec 10, 2021

ECUI supports naming styles #58084

Closed

ryzngard added UX Review Not Required UX Review Not Required and removed Needs UX Triage labels Apr 21, 2022

		@@ -0,0 +1,22 @@
		# Editorconfig settings providers and updaters

Conversation

jmarolf commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoeRobich Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Youssef1313 left a comment

Choose a reason for hiding this comment

Uh oh!

ryzngard commented Dec 2, 2021

Uh oh!

Youssef1313 commented Dec 2, 2021

Uh oh!

jmarolf commented Dec 2, 2021

Uh oh!

Youssef1313 Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

jmarolf Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarolf commented Dec 2, 2021

Uh oh!

ryzngard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryzngard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryzngard Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

ryzngard Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryzngard Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

ryzngard left a comment

Choose a reason for hiding this comment

Uh oh!

ryzngard Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmarolf commented Dec 2, 2021 •

edited

Loading