MRTK should use code analysis tools during PR validation to ensure nit critique level code quality

MixedRealityToolkit / MixedRealityToolkit-Unity

This repository holds the third generation of the Mixed Reality Toolkit for Unity. The latest version of the MRTK can be found here.

BSD 3-Clause "New" or "Revised" License

381 stars 99 forks source link

MRTK should use code analysis tools during PR validation to ensure nit critique level code quality #322

Open IssueSyncBot opened 1 year ago

IssueSyncBot commented 1 year ago

Original issue opened by:

@chrisfromwork

Overview

A lot of pull requests appear to get checked in with less than ideal coding standards: 1) Todos are left in the code with no associated issue id 2) Large blocks of commented out code are checked with no additional context provided 3) Naming choices for classes/fields vary drastically across the code base 4) Serialized fields don't end up with tooltips 5) public fields/types don't end up with summary information 6) Additional spaces/lines are added for no apparent reason

We should add a code analysis pass to our PR validation pipeline that assesses that new pr's adhere to coding standards. We should also add formal documentation calling out coding standards.

ISSUE MIGRATION

Issue migrated from: https://github.com/microsoft/MixedRealityToolkit-Unity/issues/3770

IssueSyncBot commented 1 year ago

Original comment by:

polar-kev

Things we'd like to keep from this suggestion:

Todos are left in the code with no associated issue id
Serialized fields don't end up with tooltips
Additional spaces/lines are added for no apparent reason
Anything that's public in documentation should have a summary tag

IssueSyncBot commented 1 year ago

Original comment by:

keveleigh

We might be able to use the tools described on https://docs.microsoft.com/en-us/visualstudio/code-quality/roslyn-analyzers-overview?view=vs-2019

IssueSyncBot commented 1 year ago

Original comment by:

david-c-kline

This will allow for an automated first review for PRs. We should consider cleaning up simple issues (whitespace, etc.) and mark "change requested" beyond those.

IssueSyncBot commented 1 year ago

Original comment by:

Zee2

Working on #10726 .

IssueSyncBot commented 1 year ago

Original comment by:

AMollis

Zee2's original work to add github/super-linter was removed in PR #11084 with the following comment from Zee2:

So, turns out super-linter can't lint PRs from forks and publish the results, at all :( https://github.com/github/super-linter/issues/3419

Another linting solution needs to be utilized here, one that is capable of executing and publishing results for PRs originating from forks.