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.

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

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

Open
leotsarev opened this issue Mar 4, 2018 · 28 comments · May be fixed by #54859
Open

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

leotsarev opened this issue Mar 4, 2018 · 28 comments · May be fixed by #54859
Assignees
Labels
Area-IDE Feature Request help wanted The issue is "up for grabs" - add a comment if you are interested in working on it
Milestone

Comments

@leotsarev
Copy link
Contributor

Version Used: 15.5.5

Steps to Reproduce:

enum Some {
   First,
   Second
}

Expected Behavior:
I have an .editorconfig option to force developer to add comma after Second. It's useful, because it allows easy copy/paste and reordering member, and most important, when somebody else will add Third member, line with "Second" won't be changed and won't appear in comparsions.

Actual Behavior:
I have no way to force to add trailing comma.

P.S. Sorry for my English not being 100% sharp.

@jinujoseph jinujoseph added this to the Unknown milestone Apr 20, 2018
@heaths
Copy link

heaths commented May 31, 2019

Is this still being considered? Was thinking about writing a rule to check for this, but if this is the right repo in which to implement this I'd be interested in submitting a PR here instead.

@mavasani mavasani added the Need Design Review The end user experience design needs to be reviewed and approved. label May 31, 2019
@mavasani
Copy link
Contributor

Additional context from @AArnott

There is a StyleCop.Analyzer that already does this:
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1413.md

But it would be nice to add it to .editorconfig as well so that code generators can respect it.

@sharwell
Copy link
Member

sharwell commented Jul 6, 2019

Design review conclusion: it makes sense to add an editorconfig option to include this comma, which the various code generation features would then account for. We would also accept a pull request implementing an analyzer and code fix for this option, but the main item to implement first is the editorconfig option which is used by the syntax generator for code fixes and refactorings.

@sharwell sharwell added help wanted The issue is "up for grabs" - add a comment if you are interested in working on it and removed Need Design Review The end user experience design needs to be reviewed and approved. labels Jul 6, 2019
@arch-stack
Copy link

A year later, did this get anywhere?

@sharwell
Copy link
Member

@arch-stack this item has not been implemented so far. It's still up-for-grabs if someone in the community is available to contribute the implementation, but we don't currently have it scheduled to complete ourselves.

@heaths
Copy link

heaths commented Jul 29, 2020

Looking at some examples of analyzers and code fixes you have, I'm interested. Seems the idea is to write an abstract base class that does the basic work, then pass in whatever language-specific tokenizers, etc., are needed. Though, in this case, after some doc lookups and experimentation, VB doesn't support trailing commas on enums (well, no commas anyway), collection initializers, or object initializers (the latter of which uses With anyway). So just a standalone class in C#?

@CyrusNajmabadi
Copy link
Member

Yes, this would only be needed/implemented for C#.

@arch-stack
Copy link

I'd be interested in trying to figure this out but it's a huge uphill learning experience for me. Wouldn't even know where to start

@CyrusNajmabadi
Copy link
Member

@arch-stack I'd recommnd starting with something smaller first just to get a hang of the basics of roslyn (i.e. enlisting, building, modifying, testing, pull-requests, etc). When comfortable there, moving to something like this would be a lot easier :)

@sharwell
Copy link
Member

sharwell commented Jul 30, 2020

@arch-stack this is a little more challenging than some of our "first time" fixes, but probably still can be done. I would expect the following sequence would be the most successful:

  1. Define the option, likely in CSharpCodeStyleOptions (does not need to update the corresponding UI as part of this step)
  2. Update SyntaxGenerator to account for the option, and add some tests for it
  3. Run all the tests and see what fails (if anything)

Once these three steps are done, we have the option of either merging the change as a first step and/or providing guidance on which feature(s) should be tackled next.

@heaths
Copy link

heaths commented Aug 1, 2020

I've got a start on this but wondering if we want the granularity of separate options per enum, object initializer, and collection initializer. I'm assuming so for now since existing options (related or otherwise) are granular. Or just a single option like UseTrailingCommasInMultiLineInitializers?

@CyrusNajmabadi
Copy link
Member

With formatting options we generally separate out expressions versus declarations. So I would do the same here. Two options. One for trailing commas in expressions, one for declarations.

@Redageddon

This comment has been minimized.

@sharwell
Copy link
Member

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

@jzabroski
Copy link

@CyrusNajmabadi If you're going to attempt this, please also update https://github.com/dotnet/docs/blob/master/docs/fundamentals/code-analysis/style-rules/formatting-rules.md to reflect this new feature for the .editorconfig.

I requested this previously in: dotnet/docs#22120

@jzabroski
Copy link

Also, yes, I don't think we need two options. IMHO, the whole point of this rule is so that diffs are easier to read.

@Youssef1313 Youssef1313 linked a pull request Jul 16, 2021 that will close this issue
5 tasks
@NickCraver
Copy link
Member

For what it's worth, we'd use this in every repo for a cleaner history - arrived at this issue when looking for the functionality.

@eatdrinksleepcode
Copy link

@heaths were you able to make any progress on this?

I'm starting on a new repo where I would like to have this option, and may be willing to implement it.

@heaths
Copy link

heaths commented Feb 15, 2022

Sorry, no. Other shiny things grabbed my attention. Have at it. 🙂

@davidschwegler
Copy link

Would be super awesome if you could do this @eatdrinksleepcode ! ☕

@jasonmalinowski
Copy link
Member

@eatdrinksleepcode Did you by chance start this? We're thinking about potentially picking this up directly on our team, but wouldn't want to preempt you if you were making progress.

@eatdrinksleepcode
Copy link

@jasonmalinowski I have not made any progress on it. Other shiny things also grabbed my attention :) Please, please do add it.

@Youssef1313
Copy link
Member

I've already a PR out, which I hope gets merged soon.

@jasonmalinowski
Copy link
Member

@Youssef1313 Ah, not sure why we didn't assign this to you then!

@QuintinWillison
Copy link

It would be lovely to see this land someday. As someone coming back to C# after much time away playing with other languages/frameworks, it's surprising that this isn't easy to enforce in a C# codebase (e.g. like it is with ESLint's comma-dangle rule). Source of much bike-shedding.

@CyrusNajmabadi
Copy link
Member

@QuintinWillison we'd welcome a PR :-)

@QuintinWillison
Copy link

@CyrusNajmabadi I wish I had enough free hours in my day to even imagine getting involved that deeply! 😆 ... but, in all seriousness, having noticed @Youssef1313 had already submitted one (#54859) then I was hoping my message might provide a nudge in the right direction, for it's not obvious (from a distance) as to what's blocking this other than time/availability of those already involved.

Also, open source is lovely and all that. A "foundation" sounds great, too... but don't Microsoft ultimately earn dollar from this stuff, on numerous levels of aggregation? Feels like tools being left to languish to me by the corporation/foundation, not to mention the work of open source contributors like Youssef clearly not being valued enough by that corporate/foundation to be carried over the line and merged. It all feels somewhat under resourced and ultimately pretty slapdash to me. 🤷

@CyrusNajmabadi
Copy link
Member

but don't Microsoft ultimately earn dollar from this stuff, on numerous levels of aggregation?

Sure? But that doesn't mean we have teh resources to address the 8000 issues files against this repo that people want. Ultimately, we are a team with a set of time and resource constraints, like anything else. And we make decisions on where to allocate those resources. If that allocation doesn't align with the allocation you would prefer, open source provides an avenue for you to get involved and make things happen you find more important than what is on our current roster.

Feels like tools being left to languish to me by the corporation/foundation

I don't know what the basis of this is. We've fixed literally thousands of bugs, and continue to drive new features, quality, scalability, performance and more that our ecosystem has been strongly asking for. We will not get to everything that everyone wants though. And expecting that will only make you disappointed. However unlike the closed source days, where if we didn't do something you were SOL, you now you have the option to get involved and get changes that are important to you.

mention the work of open source contributors like Youssef clearly not being valued enough

I don't know what you mean. Youssef has the most contributions to roslyn accepted from an external contributor than anyone else. He's worked on the IDE, on the Compiler and on the Language itself. We value his contributions enormously, and we work with him in close partnership here and in our other communities (like Discord). But we also value when he has his own stuff to take care of, and we will happily work with him more if he has more time to give to the Roslyn project.

and ultimately pretty slapdash to me.

We'd welcome your help and contributions here. If you're not happy that we're not prioritizing though the set of items you think are more important to you, i cannot help you with that. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Feature Request help wanted The issue is "up for grabs" - add a comment if you are interested in working on it
Projects
Status: Complete
Status: InQueue
Development

Successfully merging a pull request may close this issue.