Add prefer trailing commas code-style option #54859

Youssef1313 · 2021-07-16T06:50:13Z

What are other code generations that should respect this option?
CSharpUseObjectInitializerCodeFixProvider should respect this option
CSharpUseCollectionInitializerCodeFixProvider should respect this option.
Revise CSharpPopulateSwitchExpressionCodeFixProvider
Revise ConvertSwitchStatementToExpression
Address Implement Initializer expression wrappers #39878 (comment)

Youssef1313 · 2021-07-16T06:53:18Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs

@@ -295,6 +295,12 @@ private static Option2<CodeStyleOption2<AddImportPlacement>> CreateUsingDirectiv
                EditorConfigStorageLocation.ForBoolCodeStyleOption("csharp_style_allow_blank_line_after_colon_in_constructor_initializer_experimental", CodeStyleOptions2.TrueWithSilentEnforcement),
                new RoamingProfileStorageLocation($"TextEditor.CSharp.Specific.{nameof(AllowBlankLineAfterColonInConstructorInitializer)}")});

+        public static readonly Option2<CodeStyleOption2<bool>> PreferTrailingCommas = CreateOption(
+            CSharpCodeStyleOptionGroups.ExpressionLevelPreferences, nameof(PreferTrailingCommas),


I'm not sure what is the suitable CSharpCodeStyleOptionGroups to use here.

Youssef1313 · 2021-07-16T07:50:22Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs

@@ -295,6 +295,12 @@ private static Option2<CodeStyleOption2<AddImportPlacement>> CreateUsingDirectiv
                EditorConfigStorageLocation.ForBoolCodeStyleOption("csharp_style_allow_blank_line_after_colon_in_constructor_initializer_experimental", CodeStyleOptions2.TrueWithSilentEnforcement),
                new RoamingProfileStorageLocation($"TextEditor.CSharp.Specific.{nameof(AllowBlankLineAfterColonInConstructorInitializer)}")});

+        public static readonly Option2<CodeStyleOption2<bool>> PreferTrailingCommas = CreateOption(
+            CSharpCodeStyleOptionGroups.ExpressionLevelPreferences, nameof(PreferTrailingCommas),
+            defaultValue: s_trueWithSuggestionEnforcement,


Should this be true or false?

this should definitely be false.

and it should be silent.

I would expect true/silent. This is a maintainability improvement, not a readability improvement (it improves the quality of git blame over time).

i'm somewhat amenable to that.. i do think 'silent' has to be case here.

true/silent makes the most sense.

Although, I do think it improves readability (for me personally), in that it helps clarify it is an existentially qualified collection. My mind reads opening line { with a series of lines ending in , followed by a matching } as one giant math expression "construct collection with these members..."

Youssef1313 · 2022-01-18T18:39:14Z

I have dropped this for long. I'll get to it back as soon as I can.

src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs

Youssef1313 · 2022-01-18T20:32:30Z

@CyrusNajmabadi Can you take a look please? Thanks!

CyrusNajmabadi · 2022-01-18T20:34:28Z

src/EditorFeatures/CSharpTest/Diagnostics/GenerateEnumMember/GenerateEnumMemberTests.cs

@@ -1785,7 +1785,7 @@ void Main()
 enum Color : ulong
 {
    Green = 1UL << 0,
-    Blue = 1UL << 1
+    Blue = 1UL << 1,


this appears to eb a regression in all these tests. i would expect nothing to change unless the test explicitly turned this option on.

This is impacted by the ongoing discussion in https://github.com/dotnet/roslyn/pull/54859/files#r671040273

CyrusNajmabadi · 2022-01-18T20:35:53Z

...o/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs

@@ -151,6 +151,7 @@ private IEnumerable<CodeStyleSetting> GetExpressionCodeStyleOptions(AnalyzerConf
            yield return CodeStyleSetting.Create(CSharpCodeStyleOptions.PreferRangeOperator, description: ServicesVSResources.Prefer_range_operator, editorConfigOptions, visualStudioOptions, updaterService, FileName);
            yield return CodeStyleSetting.Create(CSharpCodeStyleOptions.ImplicitObjectCreationWhenTypeIsApparent, description: CSharpVSResources.Prefer_implicit_object_creation_when_type_is_apparent, editorConfigOptions, visualStudioOptions, updaterService, FileName);
            yield return CodeStyleSetting.Create(CSharpCodeStyleOptions.PreferTupleSwap, description: ServicesVSResources.Prefer_tuple_swap, editorConfigOptions, visualStudioOptions, updaterService, FileName);
+            yield return CodeStyleSetting.Create(CSharpCodeStyleOptions.PreferTrailingCommas, description: ServicesVSResources.Prefer_trailing_commas, editorConfigOptions, visualStudioOptions, updaterService, FileName);


hrmm... i don't think this is an expression-style... This is all for declarations (or maybe variables). consider putting in the variable section? or making a new section?

@CyrusNajmabadi Actually even those in GetVariableCodeStyleOptions have CSharpCodeStyleOptionGroups.ExpressionLevelPreferences. I'm not sure what's the suitable group here.

CyrusNajmabadi · 2022-01-18T20:40:57Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs

+        public static readonly Option2<CodeStyleOption2<bool>> PreferTrailingCommas = CreateOption(
+            CSharpCodeStyleOptionGroups.ExpressionLevelPreferences, nameof(PreferTrailingCommas),
+            defaultValue: s_trueWithSuggestionEnforcement,
+            "csharp_style_prefer_trailing_commas",


i think it shoudl likely be csharp_style_prefer_trailing_comma. @sharwell thoughts? plural on singular here? We don't say prefer_switch_expressions for example, just prefer_switch_expression. And you can't have x,,, and this doesn't impact anything but the last member (since the other members must have a comma already). so it's really about a singular trailing comma in these lists.

--

i'm also somewhat on the fence here if we should say csharp_style_prefer_trailing_enum_comma. The reason here is that we have proposals (i think i'm championing one) for things like a trailing comma in a parameter list. i'm wary about having a single option now that some users might not want to apply to different constructs.

@CyrusNajmabadi Per @sharwell's comment on the original issue:

it makes sense to add an editorconfig option to include this comma, which the various code generation features would then account for.

The "various code generation features" part implies that a single option is needed for everything. But I'm open to change.

reading through the critical decision was:

@CyrusNajmabadi it seems like only one option would be needed here. I don't think StyleCop Analyzers ever had a request to split the single option in two, and even if it did it would be an extreme minority scenario.

I'm hesitantly ok with this. However, if we have this, we need to update more than just the enum generator here. We also need to check that object/collection-initializers and arrays are generated with this and checked for this.

Also, switch/with expressions.

@CyrusNajmabadi Can you point me to where object/collection initializers and switch/with expressions generators are?

CyrusNajmabadi

some changes def need to happen. tagging @sharwell for some thoughts (esp. about the future).

CyrusNajmabadi · 2022-01-18T21:00:03Z

Leaving an analyzer and a codefix as follow-up for now.

i think this woudl be required as part of this. note: you can put this in a feature branch if you want. but to be merged into main, we would need teh analzyer/fixer.

CyrusNajmabadi · 2022-01-18T21:01:51Z

i'm hesitant about true/false being enough here. Say i'm a user that likes new Point { X = 1, Y = 2 }, but also want:

new Point
{
    X = 1,
    Y = 2,
}

it doesn't feel like i have an approach that allow me to specify this. Note: this isn't speculative. I am this user :D

CyrusNajmabadi · 2022-01-18T21:11:40Z

@sharwell i'd like your thoughts on #54859 (comment). I actually feel pretty darn strongly about this :)

sharwell · 2022-01-18T21:33:37Z

@sharwell i'd like your thoughts on #54859 (comment). I actually feel pretty darn strongly about this :)

This style applies if and only if the ,, if present, will be the final token on its line. See also SA1413 (Use trailing commas in multi-line initializers). Ideally the wrapping refactoring can account for this to add/remove the trailing , to retain adherence to this rule.

CyrusNajmabadi · 2022-01-18T21:51:48Z

This style applies if and only if the ,, if present, will be the final token on its line.

That is definitely fine with me. maybe style_prefer_trailing_comma_on_line? or maybe we can just doc.

CyrusNajmabadi · 2022-01-18T21:54:52Z

src/Workspaces/CSharp/Portable/CodeGeneration/EnumMemberGenerator.cs

+
+            if (options.Preferences.Options.GetOption(CSharpCodeStyleOptions.PreferTrailingCommas).Value)
+            {
+                members.Add(SyntaxFactory.Token(SyntaxKind.CommaToken));
            }


i actually have mixed feelings about this approach. i actually think the generator should only do this if generating the first item. if generating a followup item it should take a 'when in rome' aproach where it follows what the existing code does.

@CyrusNajmabadi Per my understanding of a previous conversation with @sharwell, code generation should adhere to code-style options when possible. If the user doesn't like the style, he should change the option value.

Some StyleCop Analyzers options have a tri-state true/false/preserve, where preserve means the current state should be followed when adding new items to the same location. The problem with a preserve value is it doesn't allow the user to also express a preference for the behavior when creating a brand new collection.

I lean towards following the defined code style in this case.

@sharwell The false value for analyzers in almost all code-style options means "preserve" (never report diagnostic). It doesn't enforce the opposite style. It's more of enable/disable rather than a real true/false.

Youssef1313 · 2022-06-24T13:10:10Z

@sharwell @CyrusNajmabadi Can you take a look? I haven't yet handled all possible syntax kinds, but the general idea will be the same.

src/Analyzers/CSharp/Tests/PreferTrailingComma/PreferTrailingCommaTests.cs

sharwell · 2022-06-24T14:43:16Z

src/Analyzers/CSharp/Tests/PreferTrailingComma/PreferTrailingCommaTests.cs

+{
+    using VerifyCS = CSharpCodeFixVerifier<PreferTrailingCommaDiagnosticAnalyzer, PreferTrailingCommaCodeFixProvider>;
+
+    public class PreferTrailingCommaTests


💡 Consider reviewing the following for additional test cases:

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.Test/MaintainabilityRules/SA1413UnitTests.cs

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1413CSharp7UnitTests.cs

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp8/MaintainabilityRules/SA1413CSharp8UnitTests.cs

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/MaintainabilityRules/SA1413CSharp9UnitTests.cs

@sharwell I think I'm covering all of these scenarios (and more).
I'm moving one case that is not covered in StyleCopAnalyzers in DotNetAnalyzers/StyleCopAnalyzers#3533 :)

src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs

sharwell · 2022-06-24T14:50:00Z

src/Analyzers/CSharp/Analyzers/PreferTrailingComma/PreferTrailingCommaDiagnosticAnalyzer.cs

+        public PreferTrailingCommaDiagnosticAnalyzer() : base(
+            diagnosticId: IDEDiagnosticIds.PreferTrailingCommaDiagnosticId,
+            enforceOnBuild: EnforceOnBuildValues.PreferTrailingComma,
+            option: CSharpCodeStyleOptions.PreferTrailingComma,


😢 Every time we add an option it makes me sad.

I've found in the case of this maintainability rule, it also adds to confusion. Some users think this is a readability rule that should be configurable. However, it is not a readability rule, and mistaking the goal here will lead some users to incorrectly configure the rule.

@sharwell Are you referring to the fact that csharp_style_prefer_trailing_comma = false will disable the analyzer rather than enforcing a non-trailing comma? Or in other words, you like this code-style option to be used by code generation but not by the analyzer?

If that's the case, I definitely agree, but #60584 needs a decision.

src/Analyzers/CSharp/Analyzers/PreferTrailingComma/PreferTrailingCommaDiagnosticAnalyzer.cs

…f1313/Roslyn into trailing-comma-code-style

Youssef1313 · 2022-06-24T19:48:51Z

There are two failings tests (FixAll crashes when there are nested diagnostics). I'm not sure how to best approach this.

…-style

Youssef1313 · 2022-07-08T15:01:08Z

It looks like "Use collection initializer" uses SyntaxFactory to produce the initializer node, where the start opening curly brace { has elastic trivia. Then, at some point (after the codefix is done, possibly the formatter) the { may or may not be followed by a new line.

Because the post processing behavior isn't consistent (sometimes a new line is put after { and sometimes not), the logic of always adding , for this codefix isn't good.

~~Should I force a new line after the {?~~ (That too seems unreliable)

It's currently unclear to me under what conditions the (formatter?) doesn't add a new line.

Youssef1313 · 2022-07-08T15:21:06Z

I can just leave collection/object initializer codefixes as they was. Is that okay? @CyrusNajmabadi @sharwell

Add prefer trailing commas code-style option

1c5303d

dotnet-issue-labeler bot added the Area-IDE label Jul 16, 2021

Simplify

0d6b5bd

Youssef1313 commented Jul 16, 2021

View reviewed changes

Youssef1313 added 3 commits January 18, 2022 20:56

Merge branch 'main' into trailing-comma-code-style

3679d87

VS and editorconfig

2620913

More fixes

593f10f

Youssef1313 marked this pull request as ready for review January 18, 2022 19:26

Youssef1313 requested a review from a team as a code owner January 18, 2022 19:26

Update xlf

94eb765

Youssef1313 commented Jan 18, 2022

View reviewed changes

src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs Outdated Show resolved Hide resolved

Update src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs

6943fb0

CyrusNajmabadi reviewed Jan 18, 2022

View reviewed changes

CyrusNajmabadi requested changes Jan 18, 2022

View reviewed changes

CyrusNajmabadi reviewed Jan 18, 2022

View reviewed changes

Youssef1313 added 4 commits February 5, 2022 19:29

Singular comma

200863c

Merge branch 'main' into trailing-comma-code-style

0118f63

Fix error

f5bda13

Missing commit

04a6ed0

runfoapp bot mentioned this pull request Feb 7, 2022

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

Youssef1313 requested review from sharwell and CyrusNajmabadi June 24, 2022 13:09

Youssef1313 added 2 commits June 24, 2022 16:10

Fix incorrectly resolved conflicts

c62f326

Update ServicesVSResources.ru.xlf

c79ba3b

sharwell marked this pull request as draft June 24, 2022 14:46

sharwell reviewed Jun 24, 2022

View reviewed changes

Youssef1313 added 4 commits June 24, 2022 21:18

More work.. Nested diagnostics fixall is failing..

effe159

Add empty enum test

cc0e20a

Update ServicesVSResources.tr.xlf

cce88be

Merge branch 'trailing-comma-code-style' of https://GitHub.com/Yousse…

7c4b00d

…f1313/Roslyn into trailing-comma-code-style

Youssef1313 requested a review from sharwell June 24, 2022 19:48

Youssef1313 marked this pull request as ready for review June 24, 2022 19:48

Youssef1313 added 3 commits June 24, 2022 22:13

Update ServicesVSResources.zh-Hans.xlf

b317c71

Fix failing tests

84b1086

Support prefer trailing comma in use object initializer

6fa0027

Youssef1313 mentioned this pull request Jun 25, 2022

Code Style for trailing commas #21784

Open

Youssef1313 added 3 commits June 25, 2022 13:43

Support prefer trailing comma in use collection initializer

23fa534

Few fixes

9c9cb3f

Fix build

7b7412e

jasonmalinowski assigned sharwell and CyrusNajmabadi Jun 27, 2022

Youssef1313 added 2 commits July 8, 2022 16:13

Merge remote-tracking branch 'upstream/main' into trailing-comma-code…

fd8489e

…-style

Fix formatting

9f16809

Add test

499ab63

QuintinWillison mentioned this pull request Jun 5, 2023

C# - .editorconfig option for force trailing comma #25206

Open

sharwell mentioned this pull request Aug 9, 2023

Add test demonstrating how trivia is handled #69448

Merged

Add prefer trailing commas code-style option #54859

Are you sure you want to change the base?

Add prefer trailing commas code-style option #54859

Conversation

Youssef1313 commented Jul 16, 2021 • 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

sharwell Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented Jan 18, 2022

Youssef1313 commented Jan 18, 2022

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

Youssef1313 Jan 18, 2022 • 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

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 18, 2022

CyrusNajmabadi commented Jan 18, 2022

CyrusNajmabadi commented Jan 18, 2022

sharwell commented Jan 18, 2022 • edited Loading

CyrusNajmabadi commented Jan 18, 2022

Choose a reason for hiding this comment

Youssef1313 Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented Jun 24, 2022

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

Youssef1313 commented Jun 24, 2022

Youssef1313 commented Jul 8, 2022 • edited Loading

Youssef1313 commented Jul 8, 2022

Youssef1313 commented Jul 16, 2021 •

edited

Loading

sharwell Jan 18, 2022 •

edited

Loading

Youssef1313 Jan 18, 2022 •

edited

Loading

sharwell commented Jan 18, 2022 •

edited

Loading

Youssef1313 Feb 5, 2022 •

edited

Loading

Youssef1313 commented Jul 8, 2022 •

edited

Loading