Enable nullable, analyzers, and code style enforcement - #37
Merged
Conversation
LangVersion=latest is what makes the rest possible: net481 and netstandard2.0 default to C# 7.3, and Nullable=enable is a hard build error (CS8630) below C# 8. Raising it also lets IDE0161 reach the .NET Framework examples, so file-scoped namespaces finally apply everywhere. AnalysisLevel is latest-recommended rather than latest-all. The five suppressed rules are documented inline; the theme is that ASP.NET dictates certain names, and logging-performance boilerplate belongs in production code rather than in a ten-line teaching sample. NU1900 is excluded from TreatWarningsAsErrors. It reports that the vulnerability audit feed was unreachable, which shouldn't fail a build. Part of #32
Mechanical follow-on from raising LangVersion. dotnet format converted the 57 remaining block-scoped namespace files; the six it skipped are tool-generated designer and WCF proxy files, which Roslyn excludes from style rules by design. Also removes 25 usings that ImplicitUsings already provides. Part of #32
Mostly narrow: null-conditional where a value really can be absent, sealing internal types with no subtypes, and PascalCase logging placeholders. The null-forgiving operators in the Web Forms page, the MVC action filter, and the Global.asax container provider all sit on values Autofac property injection or application startup guarantees. Two are more than cosmetic. The multitenant console app held its container and tenant strategy in nullable statics assigned from Main, which meant every use needed a null check the design never actually allowed; they are now readonly fields initialized where they're declared, with the strategy passed in rather than read back off a field. And the MVC actions get [HttpGet], because without it they also accept POST, which is what CA3147 was objecting to. Also drops the last ReSharper suppression comment. Part of #32
Both RootNamespace and AssemblyName said MvcExample, so the Web Forms sample built an assembly named after an unrelated project while every type in it, and every Inherits attribute pointing at those types, used WebFormsExample. Part of #32
9 tasks
Matches the Source.ruleset pattern the library repos use. The previous NoWarn was also far too broad: it suppressed five rules across all fifteen projects when only five projects needed anything, so a real CA1716 or CA5368 in a console example would have gone unreported. Splitting ASP.NET out of the common set is what the measurements asked for. CA1707, CA1716 and CA5368 only fire where System.Web dictates a type or handler name, and CA1848 and CA1873 only where ASP.NET Core logging is in play. None of them can occur in a plain console sample. Source.ruleset therefore carries no deviations yet and exists as the baseline every project inherits; AspNet.ruleset includes it and adds the eight hosted projects' exemptions. Also drops the NU1900 exclusion. That warning only appears against a feed without NuGetAudit support, so suppressing it locally is a local concern rather than something the public build should carry. Part of #32
tillig
force-pushed
the
feature/code-standards
branch
from
September 1, 2026 15:53
1ecf28d to
607c2cc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #32
Proposed Changes
A new
Directory.Build.props— there was none — carryingLangVersion,Nullable,AnalysisLevel,EnforceCodeStyleInBuild, andTreatWarningsAsErrorsin Release, so policy lives in one place instead of fifteen csproj files. All 15 projects now build with zero warnings in Release, where before there were 218.Five commits, ordered so the mechanical churn is separable from the real fixes:
Directory.Build.props.LangVersion=latestis load-bearing, not cosmetic — see below.dotnet formatoutput plus a scripted using removal. Listed in.git-blame-ignore-revs.WebFormsExampleassembly name. BothRootNamespaceandAssemblyNamesaidMvcExample.Why LangVersion had to move first
net481andnetstandard2.0default to C# 7.3, andNullable=enableunder 7.3 is a hard build error (CS8630), not a warning. So a globalNullableproperty would have broken 8 of the 15 projects. RaisingLangVersionalso unblockedIDE0161, which is why file-scoped namespaces finally reach the .NET Framework examples —dotnet formathad been silently unable to touch them for four PRs. The six files still using block scope are tool-generated designer and WCF proxies, which Roslyn excludes from style rules by design.Judgment calls worth reviewing
Five suppressed rules, documented inline in the props file.
CA1707/CA1716are names ASP.NET dictates: Web Forms code-behind forDefault.aspxis_Default,AutoEventWireuprequiresPage_Load, andGlobal.asaxexpects a type calledGlobal.CA1848/CA1873are logging-performance boilerplate — right in production, pure noise wrapped around a two-line sample, and avoiding exactly that was the point of choosinglatest-recommended.CA5368wants a per-userViewStateUserKeyand these samples have no user identity to key on.CA3147was a real finding. The MVCAboutandIndexactions had no[HttpGet], so they also accepted POST. Adding the attribute is the honest fix; adding an antiforgery token to a read-only view would not have been.The multitenant console app got a small refactor. It held its container and tenant strategy in nullable statics assigned from
Main, so every use needed a null check the design never permitted. They're now readonly fields initialized at their declaration, with the strategy passed intoConfigureDependenciesinstead of read back off a field — which also resolvedCA1859.NU1900is excluded fromTreatWarningsAsErrors. It reports that the vulnerability audit feed was unreachable, and it fired locally as a 403. A build shouldn't fail because a feed rate-limited us.Verification
Zero warnings in Release across all 15 projects, and both gates pass. Every runnable example was executed, not just compiled: the Configuration example still resolves both plugins, AttributeMetadata still exercises all five metadata paths, the multitenant console app still resolves tenant 1, tenant 2, and default to distinct overrides, and all three ASP.NET Core apps still serve their endpoints. That last set mattered most for the multitenant refactor, since moving initialization out of
Mainchanges when the container is built.The six .NET Framework projects are build-verified only, as before.