write out codestyle options to include their diagnostic ids #52916

jmarolf · 2021-04-26T17:15:36Z

fixes #52720

fixes dotnet#52720

src/Analyzers/Core/Analyzers/IDEDiagnosticIdToOptionMappingHelper.cs

src/EditorFeatures/CSharpTest/EditorConfigSettings/Updater/SettingsUpdaterTests.cs

CyrusNajmabadi · 2021-04-26T17:26:07Z

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

                }

                var optionName = storageLocation.KeyName;
                var optionValue = storageLocation.GetEditorConfigStringValue(value, optionSet);
+                var language = option.IsPerLanguage ? Language.CSharp | Language.VisualBasic : Language.CSharp;


i don't really get thsi. if it's not per langauge, then i would expect it to be C#/VB (As that's waht a normal option affects). if it is per-language, then i woudl expect it to pick the language based on what language the otion is intended for.

@CyrusNajmabadi This is tracked by #52696.

This is the reverse of the normal case our options operate under. Normally you get an option and you want to know what it applies to in the context of a project. For editorconfig we have the reverse problem. How should a per-language option be persisted? Do users want their single file to have duplicate language for each language type in their project? Or do they want a unified header?

I've chosen to, where possible, write out options that can apply to both languages together.

CyrusNajmabadi · 2021-04-26T17:26:14Z

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

                if (value is ICodeStyleOption codeStyleOption && !optionValue.Contains(':'))
                {
+                    var diagnosticIds = IDEDiagnosticIdToOptionMappingHelper.GetDiagnosticIdsForOption(option, LanguageNames.CSharp); // TODO handle VB


seems important :)

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

CyrusNajmabadi · 2021-04-26T17:28:57Z

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

+                        // Verify the option name matches
+                        if (string.Equals(key, optionName, StringComparison.OrdinalIgnoreCase))
+                        {
+                            var sb = new StringBuilder($"{optionName}={optionValue}\r\n");


should we be hardcoding \r\n in these guys? my preference woudl at least be that that's a constant defined somewhere in this file, so we can tweak this at will, or use an env variable, or allow this to be passed in.

yep I have it in a todo to actually try and get the users newline preferences for this file type if possible. In the meantime the codefix for editorconfig has been writing out \r\n since mid 2019.

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

src/EditorFeatures/CSharpTest/EditorConfigSettings/Updater/SettingsUpdaterTests.cs

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

CyrusNajmabadi · 2021-04-26T17:33:45Z

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

+                    {
+                        lastValidSpecificHeaderSpanEnd = curLine;
+                    }
+                }
            }


i admit my eyes glazed over with all the string processing. i really don't have any idea if this is right or not. Note: we have a generalized pattern matching system in Roslyn for matching over sequences of elements. it might be worthwhile seeing if we could use that. but it might also be overkill.

my primary concern here is that there seems to be a bunch of complexity here that would likely be good to extrac tout into small types that are responsibel for just one thing. i.e. this is the type that just finds the relevant portions of a chunk of hte editorconfig file.

yep, I hope to do a big refactor as part of #52735 and clean all this up

CyrusNajmabadi · 2021-04-26T17:37:18Z

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

+            // Replacing characters in the header with the regex equivalent.
+            fileName = fileName.Replace(".", @"\.");
+            fileName = fileName.Replace("*", ".*");
+            fileName = fileName.Replace("/", @"\/");


this feels unsafe. lots of other filename characters would violate regex semantics.

every editorconfig parsers I am aware of also use regex for this including the one in the compiler. The correct thing to do is to unify all this parser logic with the compilers implementation.

src/Analyzers/Core/Analyzers/IDEDiagnosticIdToOptionMappingHelper.cs

src/EditorFeatures/CSharpTest/EditorConfigSettings/Updater/SettingsUpdaterTests.cs

src/EditorFeatures/Core/EditorConfigSettings/Updater/SettingsUpdateHelper.cs

write out codestyle options to include their diagnostic ids

1ddc280

fixes dotnet#52720

jmarolf requested a review from a team as a code owner April 26, 2021 17:15

dotnet-issue-labeler bot added the Area-IDE label Apr 26, 2021