Skip to content

Enable warnings as errors#91

Merged
rjmurillo merged 5 commits intomainfrom
26-enable-warnings-as-errors
Jun 19, 2024
Merged

Enable warnings as errors#91
rjmurillo merged 5 commits intomainfrom
26-enable-warnings-as-errors

Conversation

@rjmurillo
Copy link
Copy Markdown
Owner

@rjmurillo rjmurillo commented Jun 14, 2024

  • Updated .editorconfig to set policy on some duplicate warnings for TODO. Borrowed from Add more analyzers and bump Meziantou.Analzer to latest version #89 to make merging easier
  • Updated cases where nullability was incorrect (something could be passed as null but wasn't, something was marked as nullable but wasn't). Where possible added asserts so we can catch cases when they arise.
  • Removed <NoWarn> from Moq.Analyzers.csproj
  • Added <TreatWarningsAsErrors> and MSBuildTreatWarningsAsErrors elements to CodeAnalysis.props
    • Set each to true when MSBuild property $(ContinuousIntegrationBuild) is true from DotNet.ReproducibleBuilds package

@rjmurillo rjmurillo linked an issue Jun 14, 2024 that may be closed by this pull request
3 tasks
@rjmurillo rjmurillo requested a review from MattKotsenas June 14, 2024 21:34
@rjmurillo rjmurillo added this to the vNext milestone Jun 14, 2024
@rjmurillo rjmurillo marked this pull request as ready for review June 14, 2024 21:34
@rjmurillo rjmurillo enabled auto-merge (squash) June 14, 2024 21:34
Copy link
Copy Markdown
Collaborator

@MattKotsenas MattKotsenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way so that the rules the Codacity static analysis reports are driven by our .editorconfig? It's currently showing errors for stuff we've intentionally disabled (at least to date) like CLS attributes and copyright headers.

The point isn't that those rules are good or bad, it's that we've introduced multiple, conflicting sources-of-truth.

Comment thread build/targets/codeanalysis/CodeAnalysis.props Outdated
Comment thread Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodCodeFix.cs Outdated
# Conflicts:
#	.editorconfig
#	Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs
#	Source/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs
#	Source/Moq.Analyzers/Helpers.cs
#	Source/Moq.Analyzers/NoConstructorArgumentsForInterfaceMockAnalyzer.cs
@codacy-production
Copy link
Copy Markdown

codacy-production Bot commented Jun 17, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.35% (target: -1.00%) 90.48% (target: 95.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (9c4dde8) 400 371 92.75%
Head commit (db75e30) 406 (+6) 378 (+7) 93.10% (+0.35%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#91) 21 19 90.48%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences


🚀 Don’t miss a bit, follow what’s new on Codacy.

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@rjmurillo
Copy link
Copy Markdown
Owner Author

@MattKotsenas we can turn off Codacy when we get reporting of coverage up as something we can gate on. We can also check in a config file to keep the integration sane. I disabled the Sonar analyzer, so I'm really just interested in code coverage and some obvious linting things in YAML, PowerShell, and JSON files. The recommendation from Codacy is to check in the configuration for the tools used themselves and it'll pick them up.

@rjmurillo rjmurillo requested a review from MattKotsenas June 17, 2024 21:52
@rjmurillo rjmurillo added the dependencies Pull requests that update a dependency file label Jun 18, 2024
@rjmurillo rjmurillo merged commit cff3035 into main Jun 19, 2024
@rjmurillo rjmurillo deleted the 26-enable-warnings-as-errors branch June 19, 2024 15:47
@github-actions github-actions Bot added the build label Jun 24, 2024
MattKotsenas added a commit that referenced this pull request Jul 29, 2024
#91 intended to enable warnings as errors in CI. However,
`$(ContinuousIntegrationBuild)` is set by `DotNet.ReproducibleBuilds`,
and that package's .props file is imported _after_ our own
Directory.Build.props. As a result, the properties were evaluated before
they were set.

This change moves them into the .targets file as "computed properties"
in order for them to pick up the configuration from .props files. This
aligns with the [documented
guidance](https://learn.microsoft.com/en-us/visualstudio/msbuild/customize-your-build?view=vs-2022#choose-between-adding-properties-to-a-props-or-targets-file)
"Set dependent properties in .targets files, because they pick up
customizations from individual projects.". I've also filed
dotnet/reproducible-builds#45 to see if we can
make the reproducible-builds package more intuitive.

Lastly, this cleans up existing warnings:

- Fixes whitespace issues
- Suppresses StyleCop warnings about documenting public members in
tests, which already had the built-in rules suppressed
- Reorders static members to be above instance members
- Adds a single suppression for a long method to keep the change small
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build dependencies Pull requests that update a dependency file housekeeping

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable warnings as errors

2 participants