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

Replace legacy StyleCop(Plus) with StyleCopAnalyzers #16506

Merged
merged 3 commits into from May 9, 2019

Conversation

@pchote
Copy link
Member

commented May 5, 2019

This PR migrates our custom OpenRA.StyleCheck.exe / StyleCopPlus plumbing to StyleCopAnalyzers, making the most of our recent switch to the Roslyn compiler (#16345) and the csproj <PackageReference>s (#16451).

StyleCopAnalyzers has some big advantages over StyleCopPlus:

  • Actively maintained (StyleCopPlus has not been updated since 2014)
  • Doesn't need a DIY test harness to run
  • Integrates with IDEs like VS, VS:Mac, and Rider to display errors inline and allow automated fixups
  • Includes several new style checks

The checks are run by the compiler, rather than a separate task, and trigger compilation errors.
I have set this up so that the checks are only run when compiling in the Debug configuration - this avoids the compile-time overheads when building from the commandline (which uses the Release configuration), but allows these checks to run within IDEs (after switching to Debug) or from make check (which now simply compiles the solution in Debug).

Rather than try to reverse-engineer our old configuration (the StyleCopPlus UI is defunct), I started from scratch with a blank profile and disabled rules one by one until the checks passed. There were several rules that only had one or two failures, so I fixed these violations in the code rather than disabling the rules. I divided and documented the rest into categories for things we definitely want to keep disabled, vs things that we could potentially enable in the future once someone fixes the existing violations.

There are a couple of disabled rules that I strongly agree with, so I may follow this up later with one or more further PRs to fix and enable them.

Closes #14182.
#16467 will supersede a couple of style fixups, so it would be nice but not necessary to merge that first.

I expect this to be the last blocker for updating the Mod SDK to work with the recent build system / packaging changes.

pchote added 3 commits May 2, 2019
Fix a collection of minor style violations.
This enables several new StyleCopAnalyzer rules to
be enabled immediately during migration.
Replace legacy StyleCop(Plus) with StyleCopAnalyzers
Analyzers are enabled in the Debug configuration
only to avoid unnecessary overheads when compiling
normally.

@pchote pchote added this to the Next Release milestone May 5, 2019

@reaperrr reaperrr merged commit ba28286 into OpenRA:bleed May 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.