editor config naming style #15065

jmarolf · 2016-11-07T23:11:15Z

@dotnet/roslyn-ide @jasonmalinowski
Adds the following options

Naming Style Options

editorconfig name	possible values
dotnet_naming_rule.`<naming rule title>`.severity	none , suggestion , warning , error
dotnet_naming_rule.`<naming rule title>`.symbols	`<naming symbols title>`
dotnet_naming_rule.`<naming rule title>`.style	`<naming stlye title>`
dotnet_naming_symbols.`<naming symbols title>`.applicable_kinds	class,struct,interface,enum:property,method,field,event,namespace,delegate,type_parameter
dotnet_naming_symbols.`<naming symbols title>`.applicable_accessibilities	public,internal,private,protected,protected_internal
dotnet_naming_symbols.`<naming symbols title>`.required_modifiers	abstract,async,const,readonly,static
dotnet_naming_style.`<naming style title>`.required_prefix	string
dotnet_naming_style.`<naming style title>`.required_suffix	string
dotnet_naming_style.`<naming style title>`.word_separator	string
dotnet_naming_style.`<naming style title>`.capitalization	pascal_case , camel_case , first_word_upper , all_upper , all_lower

Naming Style Examples

# Example: Pascal Casing
[*.{cs,vb}]
dotnet_naming_rule.methods_and_properties_must_be_pascal_case.severity = warning
dotnet_naming_rule.methods_and_properties_must_be_pascal_case.symbols  = method_and_property_symbols
dotnet_naming_rule.methods_and_properties_must_be_pascal_case.style    = pascal_case_style

dotnet_naming_symbols.method_and_property_symbols.applicable_kinds           = method,property
dotnet_naming_symbols.method_and_property_symbols.applicable_accessibilities = *

dotnet_naming_style.pascal_case_style.capitalization = pascal_case

# Example: async methods end in Async
[*.{cs,vb}]
dotnet_naming_rule.async_methods_must_end_with_async.severity = warning
dotnet_naming_rule.async_methods_must_end_with_async.symbols  = method_symbols
dotnet_naming_rule.async_methods_must_end_with_async.style    = end_in_async_style

dotnet_naming_symbols.method_symbols.applicable_kinds   = method
dotnet_naming_symbols.method_symbols.required_modifiers = async

dotnet_naming_style.end_in_async_style.capitalization  = pascal_case
dotnet_naming_style.end_in_async_style.required_suffix = Async

# Example: public members must be capitalized
[*.{cs,vb}]
dotnet_naming_rule.public_members_must_be_capitalized.severity = warning
dotnet_naming_rule.public_members_must_be_capitalized.symbols  = public_symbols
dotnet_naming_rule.public_members_must_be_capitalized.style    = first_word_upper_case_style

dotnet_naming_symbols.public_symbols.applicable_kinds   = property,method,field,event,delegate
dotnet_naming_symbols.public_symbols.required_modifiers = public,internal,protected,protected_internal

dotnet_naming_style.first_word_upper_case_style.capitalization = first_word_upper

# Example: non-public members must be lower-case
[*.{cs,vb}]
dotnet_naming_rule.non_public_members_must_be_lower_case.severity = warning
dotnet_naming_rule.non_public_members_must_be_lower_case.symbols  = non_public_symbols
dotnet_naming_rule.non_public_members_must_be_lower_case.style    = all_lower_case_style

dotnet_naming_symbols.non_public_symbols.applicable_kinds   = property,method,field,event,delegate
dotnet_naming_symbols.non_public_symbols.required_modifiers = private

dotnet_naming_style.all_lower_case_style.capitalization = all_lower

jmarolf · 2016-11-07T23:11:48Z

test vsi please

jasonmalinowski

Many comments but a few high level concerns:

First, let's use stronger types in a lot of places. I'd happily prefer some manually created types than Tuple<string, string, string, string> that is very challenging to maintain.

Second, there's a few places where we're building caches solely based on GetHashCode which means correctness is dependent upon an implementation that is explicitly allowed to not be correct.

jasonmalinowski · 2016-11-10T19:11:21Z

src/EditorFeatures/Next/Options/EditorConfigDocumentOptionsProvider.cs

                {
                    try
                    {
-                        value = editorConfigPersistence.ParseValue(value.ToString(), option.Option.Type);
+                        value = EditorConfigNamingStyleParser.GetNamingStylesStringFromDictionary(_codingConventionSnapshot.AllRawConventions);


Should we just update EditorConfigPersistence to always be given the full set, and then the simple case just throws away everything else? That'll avoid the duplicate code here.
#Resolved

jasonmalinowski · 2016-11-10T19:12:01Z

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/EditorConfigNamingStyleParserTests.cs

+    public partial class EditorConfigNamingStyleParserTests
+    {
+
+        /// <summary>


I always think of <summary> to mean "I want this to be shown in QuickInfo". I'm not sure that applies here. #Resolved
#Resolved

jasonmalinowski · 2016-11-10T19:15:33Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.Dictionary.cs

+{
+    internal static partial class EditorConfigNamingStyleParser
+    {
+        public static IEnumerable<Tuple<string, string, Tuple<string, string, string, string>, Tuple<string, string, string, string, string>>> ParseDictionary(IReadOnlyDictionary<string, object> allRawConventions)


Please make an actual type for this, or use our new tuples or something. This makes the code very difficult to understand.

jasonmalinowski · 2016-11-10T19:16:36Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.Dictionary.cs

+        public static IEnumerable<Tuple<string, string, Tuple<string, string, string, string>, Tuple<string, string, string, string, string>>> ParseDictionary(IReadOnlyDictionary<string, object> allRawConventions)
+        {
+            var namingRules = (from kvp in allRawConventions
+                               where kvp.Key.Trim().StartsWith("dotnet_naming_rule.")


TrimStart, just to avoid extra garbage if the end has stuff?

jasonmalinowski · 2016-11-10T19:17:21Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.Dictionary.cs

+                               select kvp).ToDictionary(x => x.Key, x => x.Value);
+
+            var namingRulesSeverity = (from kvp in namingRules
+                                       where kvp.Key.Trim().EndsWith(".severity")


Consider trimming the keys once rather than doing it during each query.

jasonmalinowski · 2016-11-10T20:42:49Z

src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs


 namespace Microsoft.CodeAnalysis.CSharp.CodeStyle
 {
    internal static class CSharpCodeStyleOptions
    {
        // TODO: get sign off on public api changes.
        public static readonly Option<bool> UseVarWhenDeclaringLocals = new Option<bool>(nameof(CodeStyleOptions), nameof(UseVarWhenDeclaringLocals), defaultValue: true,
-            storageLocations: new RoamingProfileStorageLocation("TextEditor.CSharp.Specific.UseVarWhenDeclaringLocals"));
+            storageLocations: new OptionStorageLocation[]{


You don't have to name the type, which might make it a bit less verbose. Does this being params help? #Resolved

params array doesn't work with named arguments.

It can either look like this:

public static readonly Option<bool> UseVarWhenDeclaringLocals = new Option<bool>(nameof(CodeStyleOptions), nameof(UseVarWhenDeclaringLocals), defaultValue: true, storageLocations: new OptionStorageLocation[]{ new EditorConfigStorageLocation("csharp_style_var_for_locals", ParseEditorConfigCodeStyleOption), new RoamingProfileStorageLocation("TextEditor.CSharp.Specific.UseVarWhenDeclaringLocals")});

Or this:

public static readonly Option<bool> UseVarWhenDeclaringLocals = new Option<bool>(nameof(CodeStyleOptions), nameof(UseVarWhenDeclaringLocals), true, new EditorConfigStorageLocation("csharp_style_var_for_locals", ParseEditorConfigCodeStyleOption), new RoamingProfileStorageLocation("TextEditor.CSharp.Specific.UseVarWhenDeclaringLocals"));

#Resolved

I think what @jasonmalinowski meant was that you could omit OptionStorageLocation in the first one and just have new [] { ... }.

C# does not allow you to use new [] { ... } syntax if the array contains two separate types

jasonmalinowski · 2016-11-10T20:44:31Z

src/Workspaces/Core/Portable/CodeStyle/CodeStyleHelpers.cs

+
+            switch (args[1].Trim())
+            {
+                case "none": return new CodeStyleOption<bool>(value: isEnabled, notification: NotificationOption.None);


For code styles, did we ever discuss having it be more explicit to say severity=none? My worry is an option that is "false, none" isn't exactly obvious in context what that means for somebody first seeing it.

severity=none could be ambiguous to some parsers. maybe :?

csharp_style_inlined_variable_declaration = severity=none

csharp_style_inlined_variable_declaration = severity:none

jasonmalinowski · 2016-11-10T20:45:55Z

src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs


        internal static readonly PerLanguageOption<CodeStyleOption<bool>> PreferInlinedVariableDeclaration = new PerLanguageOption<CodeStyleOption<bool>>(
            nameof(CodeStyleOptions),
            nameof(PreferInlinedVariableDeclaration),
            defaultValue: TrueWithSuggestionEnforcement,
-            storageLocations: new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.PreferInlinedVariableDeclaration"));
+            storageLocations: new OptionStorageLocation[]{
+                new EditorConfigStorageLocation("csharp_style_inlined_variable_declaration", ParseEditorConfigCodeStyleOption),


Why 'csharp' in the core layer?

PreferInlinedVariableDeclaration only applies to C#. I'll move it out.

jasonmalinowski · 2016-11-10T20:46:36Z

src/Workspaces/Core/Portable/Options/NamingEditorConfigStorageLocation.cs

+
+namespace Microsoft.CodeAnalysis.Options
+{
+    internal sealed class NamingEditorConfigStorageLocation : OptionStorageLocation { }


You might want to comment why this type is there. I don't disagree with it's existence (and now want to refactor the language settings one for clarity too...) but a comment is good. #Resolved
#Resolved

jasonmalinowski · 2016-11-10T20:49:25Z

...Features/Core/Portable/Diagnostics/Analyzers/NamingStyles/SymbolAnalysisContextExtensions.cs

+            var optionSet = await options.GetDocumentOptionSetAsync(syntaxTree, cancellationToken).ConfigureAwait(false);
+            string language = context.Compilation.Language;
+            var currentValue = optionSet.GetOption(SimplificationOptions.NamingPreferences, language);
+            int hashCode = currentValue.GetHashCode();


Isn't this assuming possibly incorrect things about hash codes? #Resolved

jasonmalinowski · 2016-11-10T21:04:35Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.Dictionary.cs

+                                where kvp.Key.Trim().StartsWith("dotnet_naming_style.")
+                                select kvp).ToDictionary(x => x.Key, x => x.Value);
+
+            var namingStyleRequiredPrefix = (from kvp in namingStyles


It would seem easier to just loop through all keys, do the split you're doing around line 63, and then just switch on the last component.

Pilchie

Perf terrifies me here:

Enumerating the editorconfig dictionary a bunch of times.
Allocating tons of yield enumerators.
No caching, so we do all of this work for every symbol the analyzer runs on (hopefully every symbol in the solution soon, currently every symbol in an open file).

Pilchie · 2016-11-11T20:10:25Z

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/EditorConfigNamingStyleParserTests.cs

+            Assert.Equal("method_and_property_symbols", symbolSpec.Name);
+
+
+            Assert.Equal(2 ,symbolSpec.ApplicableSymbolKindList.Count);


I think we have AssertEx.SetEqual which might make this easier.
#Resolved

Pilchie · 2016-11-11T20:14:37Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpTypeStyleDiagnosticAnalyzerBase.cs

+            var optionSet = options.GetDocumentOptionSetAsync(syntaxTree, cancellationToken).GetAwaiter().GetResult();
+            if (optionSet == null)
+            {
+                return;


Can we at least assert or something here? It seems bad that all of our analyzers would just silently do nothing if we broke getting options.

why not WaitAndGetResult?

Pilchie · 2016-11-11T20:16:47Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.NamingRule.cs

+{
+    internal static partial class EditorConfigNamingStyleParser
+    {
+        private static bool TryGeySerializableNamingRule(


Typo: TryGet

Pilchie · 2016-11-11T20:19:15Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.NamingRule.cs

+        {
+            diagnosticSeverity = default(DiagnosticSeverity);
+            var result = (from kvp in allRawConventions
+                          where kvp.Key.Trim() == $"dotnet_naming_rule.{namingRuleName}.severity"


Isn't the whole point of a dictionary that you can look up the keys faster than iterating over the whole thing? Why not Trim while populating the dictionary, and just use TryGetValue here?

Pilchie · 2016-11-11T20:21:21Z

...ore/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.NamingStyle.cs

+        {
+            namingStyleName = null;
+            var result = (from kvp in allRawConventions
+                          where kvp.Key.Trim() == $"dotnet_naming_rule.{namingRuleName}.style"


Same comment about switching to TryGetValue

Pilchie · 2016-11-11T20:31:35Z

...atures/Core/Portable/Diagnostics/Analyzers/NamingStyles/Serialization/SymbolSpecification.cs

+
+            public bool Equals(AccessibilityKind other)
+            {
+                return this.Accessibility == other.Accessibility;


You don't test for other == null but it could be from your object.Equals override.

fixed with change to struct type

Pilchie · 2016-11-11T20:31:55Z

...atures/Core/Portable/Diagnostics/Analyzers/NamingStyles/Serialization/SymbolSpecification.cs

-            {
-                get
-                {
+            internal DeclarationModifiers Modifier {


WHAT HAVE YOU DONE?!?!

Pilchie · 2016-11-11T20:35:27Z

...Features/Core/Portable/Diagnostics/Analyzers/NamingStyles/SymbolAnalysisContextExtensions.cs

+            int hashCode = currentValue.GetHashCode();
+            var viewModel = SerializableNamingStylePreferencesInfo.FromXElement(XElement.Parse(currentValue));
+            var preferences = viewModel.GetPreferencesInfo();
+            return preferences;


It terrifies me that we don't do any caching here and instead populate all the rules from editorconfig, serialize them to xml and then deserialize back from xml ON EVERY SYMBOL?

Pilchie · 2016-11-11T20:35:43Z

src/Features/Core/Portable/Diagnostics/Analyzers/PreferFrameworkTypeDiagnosticAnalyzerBase.cs

            if (optionSet == null)
            {
                return;
            }
-
+            


Whitespace
#Resolved

Pilchie · 2016-11-11T20:41:16Z

src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs


 namespace Microsoft.CodeAnalysis.CSharp.CodeStyle
 {
    internal static class CSharpCodeStyleOptions
    {
        // TODO: get sign off on public api changes.
        public static readonly Option<bool> UseVarWhenDeclaringLocals = new Option<bool>(nameof(CodeStyleOptions), nameof(UseVarWhenDeclaringLocals), defaultValue: true,
-            storageLocations: new RoamingProfileStorageLocation("TextEditor.CSharp.Specific.UseVarWhenDeclaringLocals"));
+            storageLocations: new OptionStorageLocation[]{


I think what @jasonmalinowski meant was that you could omit OptionStorageLocation in the first one and just have new [] { ... }.

Pilchie · 2016-11-11T20:47:53Z

Oh - also, did we decide on a way to deal with asynchrony here?

CyrusNajmabadi · 2016-11-15T23:47:37Z

src/EditorFeatures/Next/Options/EditorConfigDocumentOptionsProvider.cs

                {
+                    // TODO: report this somewhere?


Yes. We can report a NFW here.
#Resolved

CyrusNajmabadi · 2016-11-15T23:49:15Z

src/Features/CSharp/Portable/InlineDeclaration/CSharpInlineDeclarationDiagnosticAnalyzer.cs

+            var options = context.Options;
+            var syntaxTree = context.Node.SyntaxTree;
+            var cancellationToken = context.CancellationToken;
+            var optionSet = options.GetDocumentOptionSetAsync(syntaxTree, cancellationToken).GetAwaiter().GetResult();


lots of duplication across all our analyzers. Can you have a single extnsion method that does this?

CyrusNajmabadi · 2016-11-15T23:49:32Z

src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs

        {
-            return (analyzerOptions as WorkspaceAnalyzerOptions)?.Workspace.Options;
+            var workspace = (analyzerOptions as WorkspaceAnalyzerOptions)?.Workspace;
+            if (workspace == null)


how does this happen?

CyrusNajmabadi · 2016-11-15T23:49:59Z

src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs

+                return null;
+            }
+
+            var documentId = workspace.CurrentSolution.GetDocumentId(syntaxTree);


Oh god... we're accessing mutable stuff in a BG analyzer? :(

CyrusNajmabadi · 2016-11-15T23:50:57Z

...ore/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.NamingStyle.cs

+            {
+                case "pascal_case": return Capitalization.PascalCase;
+                case "camel_case": return Capitalization.CamelCase;
+                case "first_word_upper": return Capitalization.FirstUpper;


should this be Capitalization.FirstWordUpper? or shoudl the string be "first_upper"?

CyrusNajmabadi · 2016-11-15T23:51:12Z

...ore/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.NamingStyle.cs

+            object result;
+            if (conventionsDictionary.TryGetValue($"dotnet_naming_style.{namingStyleName}.word_separator", out result))
+            {
+                return (string)result;


can this cast fail?

CyrusNajmabadi · 2016-11-15T23:51:25Z

...ore/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.NamingStyle.cs

+        private static string GetNamingRequiredPrefix(string namingStyleName, IReadOnlyDictionary<string, object> conventionsDictionary)
+        {
+            object result;
+            if (conventionsDictionary.TryGetValue($"dotnet_naming_style.{namingStyleName}.required_prefix", out result))


you can use out-var

CyrusNajmabadi · 2016-11-15T23:51:38Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.SymbolSpec.cs

+            var requiredModifiers = GetSymbolsRequiredModifiers(symbolSpecName, conventionsDictionary);
+
+            symbolSpec = new SymbolSpecification(
+                    Guid.NewGuid(),


indentation weird.

CyrusNajmabadi · 2016-11-15T23:52:07Z

...Core/Portable/Diagnostics/Analyzers/NamingStyles/EditorConfigNamingStyleParser.SymbolSpec.cs

+                switch (symbolSpecApplicableKind)
+                {
+                    case "class":
+                        return new[] { new SymbolKindOrTypeKind(TypeKind.Class) };


can we use immutable arrays.

can we cache these somewhere so we don't keep allocating what are essentially constants.

dpoeschl

Only minor requests. 👍

dpoeschl · 2016-12-12T22:41:14Z

...rkspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.SymbolSpec.cs

@@ -0,0 +1,245 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.


Nit: We typically use underscores in file names when just splitting up the functionality of a type into partials, and use dots when separating out nested types.

dpoeschl · 2016-12-12T22:44:20Z

...rkspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.SymbolSpec.cs

+        private static readonly SymbolKindOrTypeKind _method = new SymbolKindOrTypeKind(SymbolKind.Method);
+        private static readonly SymbolKindOrTypeKind _field = new SymbolKindOrTypeKind(SymbolKind.Field);
+        private static readonly SymbolKindOrTypeKind _event = new SymbolKindOrTypeKind(SymbolKind.Event);
+        private static readonly SymbolKindOrTypeKind _namespace = new SymbolKindOrTypeKind(SymbolKind.Namespace);


We got rid of namespaces and type parameters, at least from the UI. Did I miss removing them somewhere else that tricked you into including them here, or were you operating off the old UI?

dpoeschl · 2016-12-12T22:46:26Z

...rkspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.SymbolSpec.cs

+                    case "public":
+                        builder.Add(Accessibility.Public);
+                        break;
+                    case "internal":


Are we standardizing on "internal" for both C# and VB? The UI in VS makes the distinction, though I don't think the old serialization code made the distinction (but that didn't matter because it wasn't user-facing).

As discussed offline, maybe we can just use either language's nomenclature interchangeably.

dpoeschl · 2016-12-12T22:50:34Z

src/Workspaces/Core/Portable/NamingStyles/Serialization/SymbolSpecification.cs


 namespace Microsoft.CodeAnalysis.Diagnostics.Analyzers.NamingStyles
 {
-    internal class SymbolSpecification
+    internal partial class SymbolSpecification


Why is this partial? I'm not seeing another part.

dpoeschl · 2016-12-12T22:51:46Z

src/Workspaces/Core/Portable/NamingStyles/Serialization/SymbolSpecification.cs

+                return Modifier == other.Modifier;
+            }
+
+            public static bool operator ==(ModifierKind left, ModifierKind right)


Why do you need the operators here but not in SymbolKindOrTypeKind?

oversight. Added the operators

dpoeschl · 2016-12-12T23:03:36Z

src/Workspaces/Core/Portable/NamingStyles/NamingStylePreferencesInfo.cs

@@ -5,11 +5,11 @@

 namespace Microsoft.CodeAnalysis.Diagnostics.Analyzers.NamingStyles
 {
-    internal class NamingStylePreferencesInfo
+    internal class NamingStyleRules


Rename the file to match.

dpoeschl · 2016-12-12T23:07:46Z

...orkspaces/Core/Portable/NamingStyles/Serialization/SerializableNamingStylePreferencesInfo.cs

+    /// 2. Name Style
+    /// 3. Naming Rule (points to Symbol Specification IDs)
+    /// </summary>
+    internal class NamingStylePreferences : IEquatable<NamingStylePreferences>


Rename file to match.

jasonmalinowski

I think this will work. Many small questions, and I see a number of other comments that should be cleaned up. But those are only local fixes.

jasonmalinowski · 2016-12-14T00:48:10Z

src/Features/Core/Portable/Diagnostics/SymbolAnalysisContextExtensions.cs

+
+            var optionSet = await context.Options.GetDocumentOptionSetAsync(location.SourceTree, context.CancellationToken).ConfigureAwait(false);
+            var namimgStylePreferences = optionSet.GetOption(SimplificationOptions.NamingPreferences, context.Compilation.Language);
+            return namimgStylePreferences.GetNamingStyleRules();


I don't think this is related to your change at this point, but it's really confusing what the difference is between "preferences" and "rules"...

jasonmalinowski · 2016-12-14T00:48:49Z

src/VisualStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersister.cs

@@ -134,6 +133,19 @@ public bool TryFetch(OptionKey optionKey, out object value)
                    return false;
                }
            }
+            else if (optionKey.Option.Type ==typeof(NamingStylePreferences))


Missing space.

jasonmalinowski · 2016-12-14T00:49:38Z

src/VisualStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersister.cs

+                // We store these as strings, so deserialize
+                if (value is string serializedValue)
+                {
+                    value = NamingStylePreferences.FromXElement(XElement.Parse(serializedValue));


This should be resilient to if that parse fails. It's the responsibility of the persister to deal with it.

jasonmalinowski · 2016-12-14T00:50:18Z

src/VisualStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersister.cs

@@ -202,7 +214,18 @@ public bool TryPersist(OptionKey optionKey, object value)
                }
            }

-            this._settingManager.SetValueAsync(storageKey, value, isMachineLocal: false);
+            if (optionKey.Option.Type == typeof(NamingStylePreferences))


Put as an else if with the previous test?

jasonmalinowski · 2016-12-14T00:51:41Z

src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs

-            var options = OptionService.GetOption(SimplificationOptions.NamingPreferences, _languageName);
-            if (string.IsNullOrEmpty(options))
+            var preferencesInfo = this.OptionService.GetOption(SimplificationOptions.NamingPreferences, _languageName);
+            if (preferencesInfo == null)


"preferences" now that we dropped "info" in the type name?

jasonmalinowski · 2016-12-14T00:53:06Z

src/Workspaces/Core/Portable/Execution/AbstractOptionsSerializationService.cs

@@ -325,7 +361,7 @@ internal override IEnumerable<OptionKey> GetChangedOptions(OptionSet optionSet)
                foreach (var kvp in _values)
                {
                    var currentValue = optionSet.GetOption(kvp.Key);
-                    if (!object.Equals(currentValue, kvp.Value))
+                    if (currentValue?.Equals(kvp.Value) == false)


or just use the static object.Equals() which does this right...

jasonmalinowski · 2016-12-14T00:59:47Z

src/EditorFeatures/Next/Options/EditorConfigDocumentOptionsProvider.DocumentOptions.cs

                    {
-                        // TODO: report this somewhere?
-                        return false;
+                        return editorConfigPersistence.TryParseReadonlyDictionary(allRawConventions, option.Option.Type, out value);


Isn't the point of such a refactoring like this that instead of this having naming-styles special case knowledge, that you'd delete NamingEditorConfigStorageLocation and make it be just a new EditorConfigStorageLocation(EditorConfigNamingStyleParser.GetNamingStylesFromDictionary)?

jasonmalinowski · 2016-12-14T01:00:10Z

src/EditorFeatures/Next/Options/EditorConfigDocumentOptionsProvider.DocumentOptions.cs

                    {
-                        value = editorConfigPersistence.ParseValue(value.ToString(), option.Option.Type);


I think you've removed the only call to the public ParseValue, and you could delete it.

Pilchie

Still a little bit concerned about the defaults assumed when things can't be found/parsed, and about what string comparisons we use.

Pilchie · 2016-12-15T23:47:55Z

src/Features/Core/Portable/Diagnostics/Analyzers/NamingStyleDiagnosticAnalyzerBase.cs

@@ -5,10 +5,11 @@
 using System.Xml.Linq;
 using Microsoft.CodeAnalysis.CodeStyle;
 using Microsoft.CodeAnalysis.Simplification;
+using System.Collections.Generic;


System first.

Pilchie · 2016-12-16T00:00:55Z

src/Workspaces/Core/Portable/Execution/AbstractOptionsSerializationService.cs

@@ -290,6 +290,42 @@ private OptionSet ReadOptionFrom<T>(OptionSet options, string language, PerLangu
            return options.WithChangedOption(option, language, value);
        }

+        protected void WriteOptionTo(OptionSet options, Option<NamingStylePreferences> option, ObjectWriter writer, CancellationToken cancellationToken)


Do we need the language-neutral ones here?

Not today. But I've added them for consistency.

Pilchie · 2016-12-16T00:01:15Z

src/Workspaces/Core/Portable/Execution/AbstractOptionsSerializationService.cs

            WriteOptionTo(options, language, CodeStyleOptions.PreferCoalesceExpression, writer, cancellationToken);
            WriteOptionTo(options, language, CodeStyleOptions.PreferCollectionInitializer, writer, cancellationToken);
            WriteOptionTo(options, language, CodeStyleOptions.PreferExplicitTupleNames, writer, cancellationToken);
            WriteOptionTo(options, language, CodeStyleOptions.PreferInlinedVariableDeclaration, writer, cancellationToken);
            WriteOptionTo(options, language, CodeStyleOptions.PreferNullPropagation, writer, cancellationToken);
            WriteOptionTo(options, language, CodeStyleOptions.PreferObjectInitializer, writer, cancellationToken);
            WriteOptionTo(options, language, CodeStyleOptions.PreferThrowExpression, writer, cancellationToken);
+            WriteOptionTo(options, language, SimplificationOptions.NamingPreferences, writer, cancellationToken);


Missing before, but it didn't matter because we don't run the naming analyzer OOP?

Pilchie · 2016-12-16T00:03:02Z

src/Workspaces/Core/Portable/NamingStyles/Serialization/AccessibilityExtensions.cs

+
+        internal static Accessibility FromXElement(XElement accessibilityElement)
+        {
+            return (Accessibility)Enum.Parse(typeof(Accessibility), accessibilityElement.Value);


Do all callers handle exceptions in case the this doesn't parse (the file has been modified, etc)?

We don't do this for code styles, are you saying we should handle this for all cases where we read XML from the option persistence service? Sounds like a good idea

I've added try/catch for all the callsites that parse XElements

Pilchie · 2016-12-16T00:05:50Z

src/Workspaces/Core/Portable/NamingStyles/Serialization/SymbolSpecification.cs

+
+            public override bool Equals(object obj)
+            {
+                return Equals((ModifierKind)obj);


Should probably do an is check here.

Pilchie · 2016-12-16T00:35:16Z

...kspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser_NamingStyle.cs

+        private static Capitalization GetNamingCapitalization(string namingStyleName, IReadOnlyDictionary<string, object> conventionsDictionary)
+        {
+            var result = GetStringFromConventionsDictionary(namingStyleName, "capitalization", conventionsDictionary);
+            return ParseCapitalizationScheme(result);


This will return PascalCase if it can't be found, or isn't a legal value. Is that what we want?

Pilchie · 2016-12-16T00:37:11Z

...rkspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser_SymbolSpec.cs

+                return ImmutableArray<SymbolKindOrTypeKind>.Empty;
+            }
+
+            var builder = ArrayBuilder<SymbolKindOrTypeKind>.GetInstance();


Create the builder after the _all return.

Pilchie · 2016-12-16T00:38:12Z

...rkspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser_SymbolSpec.cs

+            var builder = ArrayBuilder<Accessibility>.GetInstance();
+            if (symbolSpecApplicableAccessibilities.Trim() == "*")
+            {
+                builder.AddRange(Accessibility.Public, Accessibility.Internal, Accessibility.Private, Accessibility.Protected, Accessibility.ProtectedOrInternal);


Cache the _all case as above?

Pilchie · 2016-12-16T00:40:34Z

...rkspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser_SymbolSpec.cs

+            return ImmutableArray<ModifierKind>.Empty;
+        }
+
+        private static readonly ModifierKind _abstractModifierKind = new ModifierKind(ModifierKindEnum.IsAbstract);


Actually - these are structs right? Do we gain anything from caching them?

Perf was not measured.

Pilchie · 2016-12-16T00:40:53Z

...rkspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser_SymbolSpec.cs

+            var builder = ArrayBuilder<ModifierKind>.GetInstance();
+            if (symbolSpecRequiredModifiers.Trim() == "*")
+            {
+                builder.AddRange(_abstractModifierKind, _asyncModifierKind, _constModifierKind, _readonlyModifierKind, _staticModifierKind);


Consider caching the * case.

Teaching abstract options serialization service about naming styles.

Pilchie is OOF

jmarolf · 2016-12-20T23:49:52Z

@srivatsn for approval. @Pilchie is off so he can't signoff but I've addressed his comments.

srivatsn · 2016-12-21T00:46:22Z

Looks like there's been a bunch of commits to address feedback. Can someone else review the recent changes in @Pilchie's absence?

Pilchie · 2016-12-21T01:42:35Z

I will be sure to re-review tomorrow morning. Sorry for the delay.

jmarolf · 2016-12-21T01:49:50Z

@Pilchie thanks!

Pilchie

This looks good to me, except the dictionary comparer issue, but we need to follow-up with the editor team about that.

Let's merge this and make any adjustment there after the fact if necessary.

jmarolf added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 7, 2016

dnfclas added the cla-already-signed label Nov 7, 2016

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch from 528fc96 to 0132d23 Compare November 10, 2016 03:02

jmarolf removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 10, 2016

jmarolf changed the title ~~editor config naming style WIP~~ editor config naming style Nov 10, 2016

jasonmalinowski requested changes Nov 10, 2016

View reviewed changes

jasonmalinowski reviewed Nov 10, 2016

View reviewed changes

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch 2 times, most recently from 76bee8b to 57526e2 Compare November 11, 2016 03:26

jmarolf mentioned this pull request Nov 11, 2016

editorconfig codestyle options #15029

Merged

Pilchie added the Area-IDE label Nov 11, 2016

Pilchie previously requested changes Nov 11, 2016

View reviewed changes

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch from 57526e2 to 26f1ba8 Compare November 11, 2016 23:54

CyrusNajmabadi reviewed Nov 15, 2016

View reviewed changes

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch from 16a271b to d1b7a09 Compare November 17, 2016 06:32

jmarolf changed the base branch from master to dev15-rc2 November 17, 2016 06:32

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch 4 times, most recently from 82cac5d to e3d55c8 Compare November 17, 2016 10:30

dpoeschl approved these changes Dec 12, 2016

View reviewed changes

jasonmalinowski approved these changes Dec 14, 2016

View reviewed changes

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch from 955be72 to e021d50 Compare December 14, 2016 06:02

Pilchie reviewed Dec 16, 2016

View reviewed changes

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch from e021d50 to 275258f Compare December 19, 2016 04:05

jmarolf added 8 commits December 18, 2016 20:14

moving naming styles types into the workspace layer.

8178d9c

Teaching abstract options serialization service about naming styles.

Implementing naming styles support for editorconfig

d70c687

responding to david's PR feedback

aec0e29

responding to Jason's feedback

a2107f6

responding to Jason's feedback part 2

1aa1518

addressing the latest set of comments from David

04621ac

addressing Jason's comments

998425e

addressing Kevin's comments

92e3d18

jmarolf force-pushed the feature/editorconfig-namingstyle-options branch from 275258f to 92e3d18 Compare December 19, 2016 04:15

jmarolf mentioned this pull request Dec 19, 2016

CodeStyle perf #15972

Merged

Pilchie approved these changes Dec 21, 2016

View reviewed changes

srivatsn added the Approved to merge label Dec 21, 2016

jmarolf merged commit c7bcaed into dotnet:master Dec 21, 2016

tannergooding mentioned this pull request Mar 10, 2017

Improved Asin and ACos dotnet/corefx#16484

Merged

RehanSaeed mentioned this pull request Mar 17, 2017

Missing Settings madskristensen/EditorConfigLanguage#10

Open

angelsix mentioned this pull request Mar 31, 2017

Complete list of .editorconfig supported names #18342

Closed

cesarsouza mentioned this pull request Aug 27, 2017

New option: ignore_empty_lines editorconfig/editorconfig#238

Open

This was referenced Jan 31, 2020

The .editorconfig should be updated to support the code style options made available in VS2017 dotnet/runtime#7602

Closed

The .editorconfig should be updated to support the code style options made available in VS2017 dotnet/runtime#20563

Closed

jmarolf deleted the feature/editorconfig-namingstyle-options branch July 13, 2020 17:37

		Assert.Equal("method_and_property_symbols", symbolSpec.Name);


		Assert.Equal(2 ,symbolSpec.ApplicableSymbolKindList.Count);

		@@ -0,0 +1,245 @@
		// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

		{
		value = editorConfigPersistence.ParseValue(value.ToString(), option.Option.Type);

editor config naming style #15065

editor config naming style #15065

Conversation

jmarolf commented Nov 7, 2016 • edited Loading

Naming Style Options

Naming Style Examples

jmarolf commented Nov 7, 2016

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski Nov 10, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

jasonmalinowski Nov 10, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski Nov 10, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

jmarolf Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski Nov 10, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

jasonmalinowski Nov 10, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie left a comment

Choose a reason for hiding this comment

Pilchie Nov 11, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie Nov 11, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie commented Nov 11, 2016

CyrusNajmabadi Nov 15, 2016 • edited by jmarolf Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie left a comment

jmarolf commented Nov 7, 2016 •

edited

Loading

jasonmalinowski Nov 10, 2016 •

edited by jmarolf

Loading

jasonmalinowski Nov 10, 2016 •

edited by jmarolf

Loading

jasonmalinowski Nov 10, 2016 •

edited by jmarolf

Loading

jmarolf Nov 11, 2016 •

edited

Loading

jasonmalinowski Nov 10, 2016 •

edited by jmarolf

Loading

jasonmalinowski Nov 10, 2016 •

edited by jmarolf

Loading

Pilchie Nov 11, 2016 •

edited by jmarolf

Loading

Pilchie Nov 11, 2016 •

edited by jmarolf

Loading

CyrusNajmabadi Nov 15, 2016 •

edited by jmarolf

Loading