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

[Feature request]: EditorConfig #3019

Open
codecooper opened this issue Aug 9, 2024 · 9 comments
Open

[Feature request]: EditorConfig #3019

codecooper opened this issue Aug 9, 2024 · 9 comments
Labels
feature suggestion stale Stale issues or pull requests

Comments

@codecooper
Copy link

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
Copy link
Collaborator

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

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

@martincostello
Copy link
Collaborator

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

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
Copy link
Collaborator

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

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
Copy link
Collaborator

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

@patrikwlund
Copy link
Contributor

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.

Copy link
Contributor

This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made.

@github-actions github-actions bot added the stale Stale issues or pull requests label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature suggestion stale Stale issues or pull requests
Projects
None yet
Development

No branches or pull requests

4 participants