dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.1k stars 4.04k forks source link

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

Open leotsarev opened 6 years ago

leotsarev commented 6 years ago

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.

heaths commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

arch-stack commented 4 years ago

A year later, did this get anywhere?

sharwell commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

arch-stack commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

You could always just upgrade to rider and use dotsettings.

sharwell commented 4 years ago

@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 commented 3 years ago

@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: https://github.com/dotnet/docs/issues/22120

jzabroski commented 3 years ago

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.

NickCraver commented 3 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

davidschwegler commented 2 years ago

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

jasonmalinowski commented 2 years ago

@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 commented 2 years ago

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

Youssef1313 commented 2 years ago

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

jasonmalinowski commented 2 years ago

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

QuintinWillison commented 1 year ago

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 commented 1 year ago

@QuintinWillison we'd welcome a PR :-)

QuintinWillison commented 1 year ago

@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 (https://github.com/dotnet/roslyn/pull/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 commented 1 year ago

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. :-/