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

Create code formatting strategy and implementation #570

Closed
MattMcL4475 opened this issue Jan 9, 2023 · 3 comments
Closed

Create code formatting strategy and implementation #570

MattMcL4475 opened this issue Jan 9, 2023 · 3 comments
Assignees
Milestone

Comments

@MattMcL4475
Copy link
Contributor

Currently, formatting of the code is not fully automated. CoA and TES use ".editorconfig".

  1. Research the best practice for C# code formatting, as used by the C# and dotnet teams. Research the most authoritative source.
  2. Implement a formatting strategy for both local dev work, and also for PRs. For example, have a look at: https://github.com/dotnet/format . Maybe create a Github action to enforce it.
  3. The strategy might consist of both developer documentation, and Github actions, and PR gates, etc.
  4. Document any missing/desired functionality, create a strategy and roadmap for the future.
@MattMcL4475 MattMcL4475 added Bug Something isn't working and removed Bug Something isn't working labels Jan 9, 2023
@yuliadub yuliadub self-assigned this Jan 10, 2023
@MattMcL4475
Copy link
Contributor Author

@ngambani ngambani added this to the next milestone Jan 20, 2023
@yuliadub
Copy link
Contributor

yuliadub commented Jan 23, 2023

Editor config is one way to enable and disable rules and their severity to maintain consistent coding styles for everyone that works in the codebase. EditorConfig settings take precedence over global VS text editor settings. Code style rules are divided into language, unnecessary code, formatting and naming rules. There are other ways to enforce rules, like rule sets or additional configuration text files. Rules sets and EditorConfig can coexist and can both configure analyzers that are build into VS or thir pary ones. Editor config only works for VS 2019 v16.3 and later.

Ways to enforce style in repos:

Find the right .editorconfig to enforce style in repos

My suggestion is to use the .NET runtime config: runtime/.editorconfig at main · dotnet/runtime (github.com)
It is also what Microsoft documentation references: .NET code style rule options - .NET | Microsoft Learn

This is the config that is recommended to be used by most and is leveraged by other big projects. There is no concept of a "shared" editorconfig that can come from a nuget, so it will be on us to see if runtime makes changes to their config that we may want to adapt over time.

A bit more verbal guidance around .net styles: runtime/coding-style.md at main · dotnet/runtime (github.com), as well as this page: C# Coding Conventions | Microsoft Learn

Enforce the rules of editor config by preventing build of project or merge of PR or both. This can be done via a few ways:

  • Run a check to see if any editor config rules are violated as a separate github action, if there are any - block PR merge. Possible checkers, can do one or a combination
    Dotnet format
    Dotnet analyzers (build it or third party), may be worth it to run some of the Roslyn analyzers based on things that needs to be checked
    Roslyn Analyzers (does use editor config in certain cases but not all)

      		• Microsoft.CodeAnalysis.NetAnalyzers run by default on net5 or later, no need to install 
      		• Microsoft.CodeQuality.Analyzers - fags unused params 
    
  • Set a flag in all projects in the solution to enforce warnings as errors, this will prevent build from building locally as well as in the PR, where merge will be blocked. There are a few categories for this
    • TreatWarningsAsErrors (will do all warning as errors)
    • EnforceCodeStyleInBuild (will do style warnings as errors, separate category that will still show warnings even when TreatWarningsAsErrors is set to true, issue exists to fix this)
    • CodeAnalysisTreatWarningsAsErrors (will do code analysts as errors)

Automate fixing violation of the rules of editor config both locally and in repo can be done in a few ways:

  • Enforce errors on build and provide guidance to developers to build and resolve all errors before submitting PR (min bar for PR creation is the code needs to build)
  • Create a script that can be run locally prior to submitting a PR to ensure new code additions follow .editorconfig guidance
  • Set up the basic Visual Studio formatting that will auto format some of the editor config rules automatically on file save. This wont catch all style errors since VS doesn’t support correcting everything . (i.e. Code Clean up)
  • Implement a git hook that checks the code before a commit is pushed to run dotnet format, this would need to be done locally on each machine. The file to run for the pre-commit hook can be part of the repo and shared amongst all developers
  • Run a subtask after merge that checks to see if there is formatting that needs to be fixed given .editorconfig, fix it automatically and create a PR. Manual approval required for it to be merged

Helpful for setting up code formatting tools: runtime/code-formatting-tools.md at a526e77b6d5fbf362cd40e442a0ac893e681448f · dotnet/runtime (github.com)

Tools that can be used for formatting but do not use editorconfig:
- Csharpier
- Style Cop Analyzers
- ReGitLint
- ReSharper or Rider
- SonarSource/sonarlint-visualstudio: SonarLint extension for VisualStudio (github.com) (not free)

@yuliadub
Copy link
Contributor

yuliadub commented Apr 18, 2023

Update on the most recent changes:

  • both repos once the PRs are merged will have a github action that runs dotnet format on PR creation and each PR update, it will fail if any dotnet format are needed. Currently this action will show up on the PR but will not block PR from merging even if it failed
  • both repos have instructions for how to add a pre-commit hook so that "dotnet format" will run on git commit. This will need to be set up manually on repos, but only once - unless the repo is cloned in a new location.
  • at this time, no changes to the editorconfig have been made. However, if we do make them in the future, it should be easy to ensure that those guidelines are being followed with the editions of the hooks and github actions that track this.

Other conclusions:

  • dotnet format is not perfect, it does miss things and doesnt always enforce what is right. If we run into issue where dotnet format is not doing something right, it is the right path to follow up with the dotnet team.

Why did this take so long:

  • there are various ways one can enforce a commit hook, however, most of those are third party libraires that have to be maintained and updated, after trying various approaches to this I have arrived to the conclusion that the simplest solution allows us to get what we need without the overhead of having to remember to manage something just to make formatting work, using something that is built into all of our dev system with Visual Studio. I was hopefully to find a better more intuitive way to do this, but after looking around again I landed that the outcome of this is simplest and best option in my opinion.

@BMurri BMurri closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants