-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Add prefer trailing commas code-style option #54859
base: main
Are you sure you want to change the base?
Changes from 6 commits
1c5303d
0d6b5bd
3679d87
2620913
593f10f
94eb765
6943fb0
200863c
0118f63
f5bda13
04a6ed0
2c46c77
6a6313c
d06994c
3d0270a
acdcbdb
4757f83
b776b61
64972d8
cb0ae2b
c62f326
c79ba3b
effe159
cc0e20a
cce88be
7c4b00d
b317c71
84b1086
6fa0027
23fa534
9c9cb3f
7b7412e
fd8489e
9f16809
499ab63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
using System.Linq; | ||
using System.Threading; | ||
using Microsoft.CodeAnalysis.CodeGeneration; | ||
using Microsoft.CodeAnalysis.CSharp.CodeStyle; | ||
using Microsoft.CodeAnalysis.CSharp.Extensions; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Shared.Utilities; | ||
|
@@ -25,22 +26,19 @@ internal static EnumDeclarationSyntax AddEnumMemberTo(EnumDeclarationSyntax dest | |
|
||
var member = GenerateEnumMemberDeclaration(enumMember, destination, options, cancellationToken); | ||
|
||
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[^1].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.Preferences.Options.GetOption(CSharpCodeStyleOptions.PreferTrailingCommas).Value) | ||
{ | ||
members.Add(SyntaxFactory.Token(SyntaxKind.CommaToken)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sharwell The |
||
|
||
return destination.EnsureOpenAndCloseBraceTokens() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -345,6 +345,12 @@ private static Option2<CodeStyleOption2<NamespaceDeclarationPreference>> CreateN | |
"csharp_style_prefer_method_group_conversion", | ||
"TextEditor.CSharp.Specific.PreferMethodGroupConversion"); | ||
|
||
public static readonly Option2<CodeStyleOption2<bool>> PreferTrailingCommas = CreateOption( | ||
CSharpCodeStyleOptionGroups.ExpressionLevelPreferences, nameof(PreferTrailingCommas), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what is the suitable |
||
defaultValue: s_trueWithSuggestionEnforcement, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be true or false? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should definitely be false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and it should be silent. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
"csharp_style_prefer_trailing_commas", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it shoudl likely be -- i'm also somewhat on the fence here if we should say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CyrusNajmabadi Per @sharwell's comment on the original issue:
The "various code generation features" part implies that a single option is needed for everything. But I'm open to change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reading through the critical decision was:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, switch/with expressions. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi Actually even those in
GetVariableCodeStyleOptions
haveCSharpCodeStyleOptionGroups.ExpressionLevelPreferences
. I'm not sure what's the suitable group here.