[RFC] Add Editorconfig to Enforce Code Quality

[SkyeHoefling]

Description of Problem

Throughout the platform there are many code quality concerns such as

• Different line endings
• spaces vs tabs
• variable declaration styles such as _myVariable vs myVariable vs MyVariable for private variables
• Curly braces on same line or new line

Everything listed above are mixed scenarios I have found throughout the platform and we don't have a strict coding standard. All of these mismatched coding styles make it difficult for contributors and the maintainers to review Pull Request. If the entire code-base is updated to a consistent coding style the maintainability of the platform will increase.

I propose we implement a coding standard and enforce it using editorconfig. This was brought up in another RFC #2454 by @mtrutledge and I think we consider implementing it to guarantee coding standards.

Proposed Solution Option 1

I propose we adopt the .NET C# coding guidelines from Microsoft that is being used in many .NET Foundation projects documented in the dotnet/corefx project.

• coding-style.md

We should be able to re-use the .editorconfig from the BCL project dotnet/corefx with minimal changes.

• .editorconfig

Proposed Solution Option 2 (If Needed, repeat for more)

There are other tools out there, from my experience editorconfig appears to be an industry standard across .NET Foundation Open Source Projects

Alternatives Researched

Open to hearing other thoughts on this

Affected version

• [x] 10.0.0 nightly build
• [x] 9.4.1 nightly build
• [x] 9.4.0 latest supported release

[valadas]

I am totally for this, the only consideration is that it will initially increase the number of warnings by a lot. So it will get worst before it gets better. So I would prefer if we can get to 0 warning (or pretty damn near) before adding this.

[EPTamminga]

I am also totally in for this. The formatting consists mostly of straightforward changes and will enhance the readability a lot. I expect that it will even help in solving the other warnings. So for me: do not wait until the warmings pretty dawn low.

[mitchelsellers]

I’m all for this, in fact there is an Existing issue from My attempts at trying to enforce.

I am concerned about the number of warnings initially though and the impacts to AzureDevOps

[SkyeHoefling]

My biggest concern isn't necessarily build warnings it is code style. As far as I understand editorconfig is focused on code consistencies and if we add an editorconfig that is what it will change. For example: I don't think editorconfig will take care of unused variables which present as a build warning.

@mitchelsellers

I’m all for this, in fact there is an Existing issue

Do you have the issue # or any associated PR # you can share? When I did a search before creating this I couldn't find anything other than the original RFC you created, which I linked at the top

[valadas]

Yeah, I think for warnings, using analyzers with a combination of editorconfig and each analyzer config, we can set what triggers and error, a warning, or nothing(just show up suggestions while editing code)...

[SkyeHoefling]

I started to research what it will take to accomplish this and it is going to be a MASSIVE pull request to solve this problem. Currently my working branch for this change has 6,350 files changed. There are lots of inconsistencies throughout the project. Making this Pull Request is only 1/2 the problem,

while an .editorconfig forces all popular editors to conform to a style guide we define, it does not prevent it. Someone can still open up Notepad or another editor and make changes that don't conform and submit a Pull Request. This will still add human error on merging a pull request with bad styles.

There are linting tools that support .editorconfig but nothing is an officially supported linting tool. I came across a tool called eclint which is a recommended linting tool. I have been using eclint to apply changes and check for problems and it appears to work pretty well but isn't really designed for a project like ours.

• eclint

Updates to My Proposal

I propose if we move forward with using .editorconfig we enforce an .editorconfig linter such as eclint that will check the code files in the project to follow our guidelines. If the linting fails the build fails immediately. Here is an example of the commands we could run

eclint check **/*.cs
eclint check **/*.css
eclint check **/*.js
eclint check **/*.csproj

Challenges

It doesn't appear that there is a perfect linter out there that will work well with our solution, but cover our major code files will be good.

Anything is better than where we are now 😁

Proof of Concept Working Branch

• ahoefling/dnn.platform - branch: editorconfig
• Compare to Release/9.4.x

[bdukes]

@ahoefling in your review of linting tools, did you look at the ReSharper Command Line Tools? They are free and can be driven by .editorconfig and tied into a CI process.

[bdukes]

It would be nice, I think, if we find an auto-fixing linter we like, to use a GitHub Action to automatically apply linting fixes to PRs, rather than just reporting a failure and making the submitter apply changes.

[SkyeHoefling]

I have not looked at ReSharper Command Line tools but we can certainly look into that. This opens a bigger question I have

Auto-Linting vs Lint Failures

Do we want a GitHub Action to auto-lint PRs or do we want it to fail the Azure DevOps build. I personally prefer the build failure as I don't 100% trust code linters. I see lots of value in using an Auto-Linter with GitHub Actions.

Complexity???

If we create a work item from this RFC and move forward I think we should break this out to smaller manageable chunks.

• Create .editorconfig
• Apply .editorconfig to all files
• Added linter to build or GitHub Actions

If we add an .editorconfig it won't automatically be applied to old files, but it will work for new files. I think this is the first step as doing this all at once is proving to be a massive change.

[bdukes]

Regarding auto-linting vs. build failures

[valadas]

This was implemented