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.

Sign up for GitHub

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

Jump to bottom

Add prefer trailing commas code-style option #54859

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Add prefer trailing commas code-style option #54859

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1c5303d
Add prefer trailing commas code-style option
Youssef1313 Jul 16, 2021
0d6b5bd
Simplify
Youssef1313 Jul 16, 2021
3679d87
Merge branch 'main' into trailing-comma-code-style
Youssef1313 Jan 18, 2022
2620913
VS and editorconfig
Youssef1313 Jan 18, 2022
593f10f
More fixes
Youssef1313 Jan 18, 2022
94eb765
Update xlf
Youssef1313 Jan 18, 2022
6943fb0
Update src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs
Youssef1313 Jan 18, 2022
200863c
Singular comma
Youssef1313 Feb 5, 2022
0118f63
Merge branch 'main' into trailing-comma-code-style
Youssef1313 Feb 5, 2022
f5bda13
Fix error
Youssef1313 Feb 5, 2022
04a6ed0
Missing commit
Youssef1313 Feb 5, 2022
2c46c77
Merge remote-tracking branch 'upstream/main' into trailing-comma-code…
Youssef1313 Jun 24, 2022
6a6313c
Adapt implementation to recent refactoring
Youssef1313 Jun 24, 2022
d06994c
Delete CSharpCodeGenerationPreferences.cs
Youssef1313 Jun 24, 2022
3d0270a
Update CSharpCodeGenerationOptions.cs
Youssef1313 Jun 24, 2022
acdcbdb
Few fixes
Youssef1313 Jun 24, 2022
4757f83
Merge branch 'trailing-comma-code-style' of https://GitHub.com/Yousse…
Youssef1313 Jun 24, 2022
b776b61
Code fix
Youssef1313 Jun 24, 2022
64972d8
More progress
Youssef1313 Jun 24, 2022
cb0ae2b
Adjust implementation
Youssef1313 Jun 24, 2022
c62f326
Fix incorrectly resolved conflicts
Youssef1313 Jun 24, 2022
c79ba3b
Update ServicesVSResources.ru.xlf
Youssef1313 Jun 24, 2022
effe159
More work.. Nested diagnostics fixall is failing..
Youssef1313 Jun 24, 2022
cc0e20a
Add empty enum test
Youssef1313 Jun 24, 2022
cce88be
Update ServicesVSResources.tr.xlf
Youssef1313 Jun 24, 2022
7c4b00d
Merge branch 'trailing-comma-code-style' of https://GitHub.com/Yousse…
Youssef1313 Jun 24, 2022
b317c71
Update ServicesVSResources.zh-Hans.xlf
Youssef1313 Jun 24, 2022
84b1086
Fix failing tests
Youssef1313 Jun 25, 2022
6fa0027
Support prefer trailing comma in use object initializer
Youssef1313 Jun 25, 2022
23fa534
Support prefer trailing comma in use collection initializer
Youssef1313 Jun 25, 2022
9c9cb3f
Few fixes
Youssef1313 Jun 25, 2022
7b7412e
Fix build
Youssef1313 Jun 25, 2022
fd8489e
Merge remote-tracking branch 'upstream/main' into trailing-comma-code…
Youssef1313 Jul 8, 2022
9f16809
Fix formatting
Youssef1313 Jul 8, 2022
499ab63
Add test
Youssef1313 Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions src/Workspaces/CSharp/Portable/CodeGeneration/EnumMemberGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Collections.Generic;
using System.Linq;
using Microsoft.CodeAnalysis.CodeGeneration;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Shared.Utilities;
Expand All @@ -26,22 +27,19 @@ internal static EnumDeclarationSyntax AddEnumMemberTo(EnumDeclarationSyntax dest

var member = GenerateEnumMemberDeclaration(enumMember, destination, options);

if (members.Count == 0)
{
members.Add(member);
}
else if (members.LastOrDefault().Kind() == SyntaxKind.CommaToken)
{
members.Add(member);
members.Add(SyntaxFactory.Token(SyntaxKind.CommaToken));
}
else
if (members.Count > 0 && !members.LastOrDefault().IsKind(SyntaxKind.CommaToken))
{
var lastMember = members.Last();
var trailingTrivia = lastMember.GetTrailingTrivia();
members[members.Count - 1] = lastMember.WithTrailingTrivia();
members.Add(SyntaxFactory.Token(SyntaxKind.CommaToken).WithTrailingTrivia(trailingTrivia));
members.Add(member);
}

members.Add(member);

if (options.Options.GetOption(CSharpCodeStyleOptions.PreferTrailingCommas).Value)
{
members.Add(SyntaxFactory.Token(SyntaxKind.CommaToken));
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Youssef1313 Youssef1313 Feb 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.


return destination.EnsureOpenAndCloseBraceTokens()
Expand Down
6 changes: 6 additions & 0 deletions ...rkspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member Author

Choose a reason for hiding this comment

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

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

defaultValue: s_trueWithSuggestionEnforcement,
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be true or false?

Copy link
Member

Choose a reason for hiding this comment

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

this should definitely be false.

Copy link
Member

Choose a reason for hiding this comment

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

and it should be silent.

Copy link
Member

@sharwell sharwell Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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..."

"csharp_style_prefer_trailing_commas",
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Youssef1313 Youssef1313 Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Also, switch/with expressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

$"TextEditor.CSharp.Specific.{nameof(PreferTrailingCommas)}");

#if false

public static readonly Option2<CodeStyleOption2<bool>> VarElsewhere = CreateOption(
Expand Down