domaindrivendev / Swashbuckle.AspNetCore

Swagger tools for documenting API's built on ASP.NET Core
MIT License
5.21k stars 1.3k forks source link

[Feature request]: EditorConfig #3019

Open codecooper opened 1 month ago

codecooper commented 1 month ago

Is your feature request related to a specific problem? Or an existing feature?

When working on the source code for Swashbuckle, I would like Visual Studio and VS Code to understand the coding standards and styles of the repository.

Describe the solution you'd like

I recommend adding an EditorConfig and configure it to enforce all coding styles and standards. This could even be added to the build steps of the projects to ensure that all braces, line breaks and other styles are enforced. This should help to remove formatting issues in code reviews for pull requests.

Additional context

No response

martincostello commented 1 month ago

We do have an EditorConfig file already.

We just don't currently enforce any style as part of the build as there's never historically been any style rules or consistency, and making everything consistent at this stage would generate a lot of code churn.

Personally, when I review things I try and go with "is it mostly equivalent with the extant local style", but suggest improvements (which is subjective I know) if a PR is going to need non-style edits anyway.

richard-collette-precisely commented 3 weeks ago

Would it make sense to run dotnet format one time and then dotnet format --verify-no-changes in the build pipeline.

martincostello commented 3 weeks ago

In my own projects I use StyleCopAnalyzers.

My main concern here is more about the churn that adopting a style linter would bring doing that one-time standardisation.

richard-collette-precisely commented 3 weeks ago

By churn do you mean the mass number of commits that would result in? If that's the case, I'm not sure what the concern is. You do it once, you know why, and you just move on from there. It's so much nicer using dotnet format (or prettier, etc.) because now you know that changes are more than inconsequential and random IDE formatting.

I use analyzers as well but those go beyond structural formatting and into the realm of naming standards and beyond.

martincostello commented 3 weeks ago

No, I mean having to touch dozens (probably hundreds?) of files in a single commit to get everything "clean" (multiple commits would be even noiser IMHO) and then people having to review that.

Personally I think the idea would be to have a style (whatever that is) and then be able to tidy things up as specific lines of code need to be touched. Essentially, eventual consistency. The issue here is just that what that style is isn't codified and there's an open question of how to enforce it in a way that isn't a surprise to committers.

I'm not sure for the size of the project and its age that it's worth at this stage going through every file adopting a specific standard at this specific moment in time.

If this project was exclusively my own and I didn't need other people to review things, then I'd have done this a while ago and just adopted style analysers. Due to that not being the case, this is why I avoided adding any style (and most of the roslyn) analysers when I overhauled the CI earlier this year. The diff would have been enormous 😅.

richard-collette-precisely commented 3 weeks ago

Absolutely the diff would be enormous, but it's done by an automated task, one time. There is nothing inherently wrong for funny with an enormous diff when every single part of that diff is machine generated, doubly if you already have unit tests.

Every time you upgrade the compiler, you are effectively doing the same thing. Regenerating your entire product, and trusting it to do so.

I do think you might be mixing in style analyzers here when they don't apply. dotnet format is just about line length, wrapping, etc. It's just applying the editorconfig, not telling you things like your private fields are missing (or not) an underscore prefix or other code styles like that. I don't even have an editorconfig in my project. I just use whatever the defaults of dotnet format are, much like just running prettier and taking the defaults. There's no squabling over setting individual settings this way.

Anyway, it's just a suggestion. Others do it and it's helpful.

martincostello commented 3 weeks ago

Regardless of whether it's automated or not, someone still has to review that diff.

patrikwlund commented 1 week ago

In order to introduce and review these kinds of very wide changes I find it easiest to apply one solution-wide code-fix per PR. Then it's quite easy to scroll through and spot oddities.

Running a full dotnet format on legacy code for the first time is bound to create unwanted changes.