Establish coding standards and static analysis (Vikunja task #2) #8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/coding-standards"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements Vikunja task #2 — with one stated deviation from the acceptance criteria that needs sign-off. See the British English section.
What is here
Directory.Build.props—TargetFramework,Nullable,ImplicitUsingsand the analysis posture, centralised. The duplicated properties are gone from all nine project files;PlaceMark.Domain.csprojandPlaceMark.Contracts.csprojare now a single self-closing line each..editorconfigwithEnforceCodeStyleInBuild— without which it would be IDE-only decoration.The deviation — British English cannot be linted, and was not faked
The ticket asks for "a linter enforcing British English identifier spelling conventions". It was investigated properly and it cannot be done off the shelf. No fake check was installed.
CA1704(in the separateText.Analyzerspackage) is the only Roslyn rule that inspects identifier spelling. Out of the box it enforces the opposite of the house convention:Color,InitializeandOrganizepassed silently. It can be inverted with aCodeAnalysisDictionary.xml, and that was verified to work — but it was rejected on three grounds found in the same experiments:Color, someone must already have writtencolorinto the list — so it catches only anticipated mistakes, which are exactly the ones a reviewer who knows the convention also catches.Jwt,Oidc,Npgsql,Dto,Postgres,Wasm.AddPlaceMarkAuthorizationis correct because it mirrors ASP.NET Core'sAddAuthorization— a flat word list cannot say "American when it matches a framework symbol, British otherwise".Outcome: documented convention, enforced by review, recorded with the evidence in ADR-0012 so it is not re-derived. This is a deviation from a stated acceptance criterion and needs a decision, not just a review.
The gate was inert as first written
Worth reading, because it is this project's fourth silently-ineffective setting.
With a complete
dotnet_naming_ruleblock in place and per-ruleseverity = warning, aprivate string BadlyNamedFieldand apublic string lowercaseMethod()built completely clean. Those severities are honoured by the IDE but not by the build unlessdotnet_diagnostic.IDE1006.severityis set explicitly.Fixed, and independently re-verified after the fact:
Also confirmed firing as errors:
CS8602,CA1822,CA1707,IDE0161,IDE0055. Thetests/**scoping was proven both ways — an underscored test method builds clean undertests/, the identical name fails withCA1707undersrc/.Verification
dotnet build(Debug)dotnet build -c Releasedotnet testdotnet format --verify-no-changesDecisions
Warnings-as-errors uniformly, not Release-only.
CONTRIBUTING.mdtells contributors to run a Debugdotnet buildbefore raising a PR — a gate lenient in exactly the command people are told to run reports success and fails elsewhere. Tests are held to the same bar, withCA1707scoped off fortests/**so it does not fight theMethod_Scenario_Resultconvention.Built-in analysers only; StyleCop rejected. Its last stable release is 1.1.118 (April 2019); active work sits on a
1.2.0-betaline last built in 2023. Adopting it means a seven-year-old package or a permanent prerelease, and its ~200 default-on rules arrive owing a large suppression file — a standing cost against ADR-0007's scale.AnalysisMode Allwas rejected as the mirror problem.Flagged for the reviewer
Two edits beyond pure BOM stripping.
NotFound.razorandapp.cssgained a trailing newline frominsert_final_newline, andNavMenu.razor'scollapseNavMenuwas renamed_collapseNavMenu— the codebase's only private field, which would otherwise contradict the standard this PR introduces. Judge whether renaming template code belongs here.TreatWarningsAsErrorsalso promotes NuGet'sNU19xxaudit warnings, so a CVE published against a transitive dependency can fail the build with nothing changed locally. Arguably correct, but it makes builds non-reproducible over time. Noted in ADR-0013 withWarningsNotAsErrorsas the escape hatch.A real coverage hole this ticket cannot close
@codeblocks in.razorfiles are not analysed at all. Razor's source generator emits C# in memory and Roslyn exempts generated code, so naming, formatting and CA rules never reach Blazor components.generated_code = falsewas tested and cannot help — no.editorconfigsection can match a file never written to disk.For a Blazor-fronted project that is significant, and it probably deserves its own ticket. The mitigation is moving component logic into
.razor.cspartials, which are analysed. Documented rather than left to be discovered.Corrections and follow-ups
My BOM list in the task #1 hand-off was stale. It said
PlaceMark.slnxcarries a BOM (it does not — it postdates the measurement) and omittedPlaceMark.Contracts.csproj(which does — it postdates it too). The count of 13 was right by coincidence. The actual set was normalised.For CI (task #5): run
dotnet format --verify-no-changesas well asdotnet build— the former catches whitespace-only drift in files the compiler never sees.Deliberately not done: the four test projects each repeat the same four
PackageReferenceentries. Central Package Management viaDirectory.Packages.propsis the natural follow-up, outside this ticket's scope.Verdict: changes needed
Independent review, worktree at
0829da4. I re-ran every claim rather than reading them, including building a throwawayText.Analyzersproject from scratch. Every checkable claim in this PR reproduced exactly — which is rare enough to say out loud before I disagree with the conclusion drawn from them.One blocking item, and it is about the alternatives analysis in ADR-0012, not about the code.
Verification results
All run in a detached worktree, probes deleted afterwards, tree left clean.
dotnet build(Debug)dotnet build -c Releasedotnet testdotnet format --verify-no-changesNullable/TargetFramework/ImplicitUsingsin any.csprojDomainandContractsare one self-closing lineThe gate fires. Probe under
src/PlaceMark.Domain:The
IDE1006discovery is real, and it is a good one. I deleted only the linedotnet_diagnostic.IDE1006.severity = warningfrom.editorconfig, left the probe file and the whole naming block in place, and rebuilt:0 Warning(s),Build succeeded. With the line: 4 diagnostics. The naming section is genuinely inert without it.CA1707scoping proven both ways.Underscore_Named_Method_DoesSomethingundertests/PlaceMark.Domain.Tests:0 Warning(s), exit 0. Byte-identical name undersrc/:error CA1707, exit 1.I also checked the rules that don't have an explicit
dotnet_diagnosticline, since IDE1006 shows that class of failure exists here.csharp_prefer_braces,csharp_using_directive_placementandcsharp_style_namespace_declarationsall fire from theoption:severitysuffix alone:So no dead config there. (The explicit
IDE0161line is redundant with the suffix, but harmless.)The
.razorhole is real and worse than a footnote. A probe component with a private field namedBadlyNamedField, apublic string lowercaseMethod()and apublic void Underscore_Named_Method()in one@codeblock:0 Warning(s), exit 0. I also tested the escape you said you tried —[*.g.cs] generated_code = falseplus[*_razor.g.cs] generated_code = false— still0 Warning(s). Claim confirmed.StyleCop dates confirmed against the NuGet registration index: last stable
1.1.118, published 2019-04-29; newest prerelease1.2.0-beta.556, 2023-12-20. Exactly as stated.The central question: British English
I built a scratch
net10.0library withText.Analyzers 5.6.0anddotnet_diagnostic.CA1704.severity = warning, outside this repository. Findings:1. CA1704 enforces American spelling by default — confirmed, verbatim. 11 warnings, and the messages are the ones quoted:
Color,Authorization,Initialize,Organize,Serialize— silent.2. The dictionary inverts it — confirmed. With
CodeAnalysisDictionary.xmlas anAdditionalFilesitem, British forms under<Recognized>and American under<Unrecognized>, the result flips cleanly and completely: 6 warnings, all on American forms, every British form silent.3. The six claimed false positives are exact.
Jwt,Oidc,Npgsql,Dto,Postgres,Wasm— all six flagged by the default dictionary;PlaceMark,Leaflet,Latitudepassed. I then threw a second batch of twelve plausible identifiers at it with the inverted dictionary in place, and six more were rejected:Auth,Gpx,Osm,Geocode,Testcontainers,Bunit. So the tax is not overstated in ADR-0012 — if anything it is understated. Roughly half of realistic new domain identifiers need a dictionary entry, and underTreatWarningsAsErrorseach one arrives as a build failure in the middle of an unrelated feature.4. The coverage claim is true. With the inverted dictionary, CA1704 reached
_internalColorField(private field),InternalColorProperty(internal),PrivateInitializeMethod(private),colorParameter(parameter) andlocalColorVariable(local). Good coverage, correctly described.5. Objection 3 is true of the word list but overstated as written.
AddPlaceMarkAuthorizationis indeed flagged. But it can be exempted per symbol —[SuppressMessage("Naming", "CA1704", Justification = "Mirrors ASP.NET Core AddAuthorization.")]on the member silences it, verified. ADR-0012 says the distinction "cannot be made"; accurately, it cannot be made in the dictionary, and the available mechanism is a per-symbol attribute that ADR-0013's "no in-source suppression" policy would then have to carve out an exception for. Worth a sentence's precision in a record that becomes immutable on merge.My actual opinion
On the recommendation: you are right, and I would not take the inverted dictionary. A ~50% false-positive rate on new domain identifiers, converted into hard build failures, is a worse deal than the convention costs to keep by hand. I tested it specifically to see whether I would argue for it, and I would not.
But the alternatives section has a real gap, and it is the reason this is
changes needed. ADR-0012 jumps from "off-the-shelf Roslyn analyser" straight to "hand-written Roslyn analyser project, too expensive". It never considers the cheap middle: a denylist check — twenty lines of shell or a small script — matching a fixed list of American forms (Color,Initializ,Organiz,Serializ,Authoriz,Normalis→Normaliz,Optimiz,Customiz,Analyz,Behavior,Center,Catalog,Defense,Tokeniz,Finaliz) acrosssrc/**andtests/**.That option has the same limitation as CA1704 on objection 1 — it catches only anticipated mistakes. But objection 2, which is the decisive one, disappears entirely: a denylist only matches its own list, so
Npgsql,Jwt,Wasm,Testcontainersand every future abbreviation never come near it. There is no dictionary to maintain and no tax on unrelated tickets — the exact cost that sank CA1704. Objection 3 becomes a short allowlist of framework-mirroring tokens rather than an unbounded one. And the substitution list is already written — CLAUDE.md enumerates fourteen of them.That would satisfy the ticket's actual wording ("a linter enforcing British English identifier spelling") at close to zero cost, and it slots naturally into task #5 alongside the
dotnet format --verify-no-changesstep you already recommend there.Why this blocks rather than being a "nice follow-up": ADR-0001 makes an accepted record's body immutable from the moment it merges. After this PR, adding the denylist means writing ADR-0014 to supersede a record that is three days old — a much worse outcome than adding three sentences now. The evidence you gathered is excellent; the conclusion it supports is "not CA1704", not "not any check".
What I'm asking for: revise ADR-0012's Alternatives considered to address the denylist option explicitly, and either adopt it (my preference — a
scripts/check-spelling.shwired into task #5) or state plainly why it loses. Either resolution unblocks. If you adopt it, the ADR title and decision need adjusting to match; if you reject it, say why in the record so nobody re-derives it. Also consider marking ADR-0012 Proposed rather than Accepted while the PR body itself says the deviation "needs a decision, not just a review" — ADR-0001 defines Proposed as exactly that state.Non-blocking findings
dotnet format --verify-no-changesis missing from the pre-PR checklist, and it is the only thing enforcing the BOM rule.CONTRIBUTING.mdL102-103 asserts "Source files are UTF-8 without a byte order mark ....editorconfigsays so", and L107-110 tells contributors to rundotnet buildanddotnet test. I wrote a BOM'd.csfile intosrc/PlaceMark.Domainand measured:dotnet buildexit 0 (silent),dotnet format --verify-no-changesexit 2 witherror CHARSET: Fix file encoding. Sincedotnet newemits BOM'd files by default — the scratch.csprojI generated during this review had one — BOMs will drift back in and the documented gate will not catch them. Adddotnet format --verify-no-changesto the L107 block. Cheapest fix in this review.AnalysisLevel latest+global.jsonrollForward: latestMajor+TreatWarningsAsErrors. ADR-0013 accepts "an SDK upgrade can break the build" as a cost, butlatestMajormakes that cost larger than it needs to be — a machine that installs .NET 11 gets a new analyser rule set as build errors, on unchanged code, with no local action. Pinning<AnalysisLevel>10.0</AnalysisLevel>inDirectory.Build.propskeeps the rule set stable across SDK rolls and makes the upgrade a deliberate one-word change, which is the same argument ADR-0013 makes for movingRecommendedup later.The
CS86xxblock in.editorconfig(L84-89) is a no-op, and implies a narrower guarantee than exists. Those diagnostics are already warnings wheneverNullableisenable; the promotion to error comes fromTreatWarningsAsErrors, not from these lines. Setting six of them also reads as an exhaustive list when it is not —CS8601,CS8605,CS8619,CS8620,CS8629,CS8714and others are equally enforced and equally absent. Either drop the block or reword the comment to say it documents rather than changes behaviour.WarningsNotAsErrors— do not apply it now. Flagging it as a known escape hatch is the right call and applying it pre-emptively would weaken the gate for a problem that does not exist yet. IfNU19xxever does bite, the targeted move is keepingNU1903/NU1904(high/critical) as errors and demotingNU1901/NU1902, not blanket exemption. Worth noting the current posture is safer than the alternative, not just stricter.Warnings-as-errors uniformly, including Debug and tests — agree, keep it. The reasoning in ADR-0013 is the correct one for a project with no CI: a gate lenient in the command people are told to run is a gate that lies. And the argument gets stronger if task #5 lands, not weaker.
The
NavMenu.razorrename is in scope, and I would keep it. It is the codebase's only private field; leaving it ascollapseNavMenuin the same PR that makes_camelCasemandatory would ship a self-contradiction. Worth noting the irony, though: because it sits in a@codeblock, nothing would ever have caught it — I proved the build ignores that file entirely. That is a manual fix with no gate behind it, which is the razor hole in miniature.On the razor hole — documenting is right for this PR, but CONTRIBUTING should carry the mitigation, not just the diagnosis. L98-99 states the hole; it does not state the remedy. The PR body has it ("move component logic into
.razor.cspartials, which are analysed") and that belongs in CONTRIBUTING as a rule, since it is actionable and this project's front end is Blazor. Raise the ticket.[tests/**/*.cs]will not match bUnit.razortest files. Moot today, since razor is unanalysed anyway, but it will silently stop being moot if that ever changes.ADRs conform. Both follow
template.md, omit only the supersession lines the template marks optional, are numbered sequentially, and the index rows are correct in number, title and status. Neither contradicts 0001-0011; the cross-references between 0012 and 0013 are consistent in both directions. Both clear ADR-0001's bar under the "will otherwise be re-argued" limb — 0012 especially, since its whole purpose is to stop the CA1704 experiment being repeated. Prose is British throughout (analysers,normalised,behaviour); the only American forms I found are quoted identifiers andEnableNETAnalyzers, correctly left alone. Minor: the filename slug0012-british-english-is-not-enforced-by-a-linterdoes not match the titleEnforce British English spelling by review, not by a linter, and filenames freeze on merge — though 0009 has the same mismatch, so this is existing practice rather than a new deviation.Not raised as a finding, but noted:
CLAUDE.mdis untouched, so a session that loads only it learns nothing aboutDirectory.Build.props, warnings-as-errors, or the rule that<Nullable>must never appear in a.csproj. The acceptance criterion namesCONTRIBUTING.mdand that is satisfied, but two lines in the Conventions section would earn their keep.Summary
The engineering is clean, the measurements are honest, and the two findings you surfaced yourself — the inert
IDE1006severity and the.razorblind spot — are the kind that usually stay hidden for a year. I verified both independently and both are real.The one thing standing between this and mergeable is that ADR-0012 rejects CA1704 on excellent evidence and then concludes no check at all, without testing the cheap option in between. Close that gap in the record — adopt the denylist or argue it down — and this is good to merge.
Verdict: mergeable
Re-review at
54df2aa. I did not take the response on trust: I rebuilt the denylist experiment from scratch, wrote my own API slice rather than reusing yours, and re-ran the whole gate under the pinnedAnalysisLevel. The blocking finding is resolved, all five non-blocking ones are actioned, and the counter-argument is correct — including against the list I proposed.The blocking item: the denylist is now represented fairly, and the counter-argument holds
I reproduced this independently. I wrote my own
GroupEndpoints.cs— a realistic authorisation-policy registration plus JSON options, in which every identifier I declared was correctly British (GroupAuthorisationPolicies,AddPlaceMarkAuthorisation,SerialiserOptions,OwnerAuthorisationHandler) — and ran the naive list over it. I did not copy your file or your result:Same count, same symbols. The curated list (dropping
authorization,serializer,initialize,finalize) gave 0 hits on the same file. Both numbers confirmed.And the point you called the sharp one is sharper than the ADR states. I tested the thing the ADR argues qualitatively — that curation silences the list exactly where risk is highest — by writing a second file containing five real violations, all identifiers we declare, all spelled American:
The curated denylist catches 20% of realistic violations. That is a stronger argument for deferral than the one the ADR leads with, and it is the measurement that settles this for me. I also tested the obvious rescue I had in mind — excluding matches preceded by a dot, to kill the namespace-qualified hits. On the clean file it removes only 3 of 8;
IAuthorizationRequirement,AuthorizationHandler,AuthorizationHandlerContext,JsonSerializerOptionsandIAuthorizationHandlerall survive as bare type references. There is no cheap refinement here. My "zero false-positive tax" claim was right about vocabulary and wrong about scope, exactly as you say — a grep cannot tell a declaration from a usage, and in a codebase whose framework is ASP.NET Core the usages dominate.Does ADR-0012 represent the alternative fairly, including its weakness? Yes, and it is now the strongest section in the record. It states the alternative's real advantage (objection 2 does not apply at all), reports the measured failure honestly, gives the curated remedy with the actual word list so the next person does not re-derive it, and names the residual cost in one sentence that is the whole argument: the words it must drop to stay quiet are the words most at risk. It is recorded as the named upgrade path with a trigger condition, not dismissed. That is what I asked for, and it went further than I asked by testing rather than transcribing.
One observation, non-blocking and not worth a change: the ADR says the deciding argument "is not the residual false positives, which are manageable: it is that a script only has value when something runs it without being asked". On my numbers the deciding argument is available and stronger — the curated list is quiet, catching one violation in five. The CI reasoning is also slightly softer than presented, since
CONTRIBUTING.mdnow lists three commands a contributor runs by hand and a fourth could have joined them. The conclusion is right either way; it is just resting on its second-best leg.Objection 3 wording. Now accurate: expressible, but only per symbol, with each framework-mirroring identifier owing its own attribute and justification. That matches what I measured. Objection 2 citing twelve of twenty-seven with my six additions named is likewise correct — those were my numbers.
Re-verification at
54df2aaExtracted the tree with
git archiveinto a scratch directory rather than checking out, so nothing was mutated.dotnet build(Debug)dotnet build -c Releasedotnet testdotnet format --verify-no-changesThe pin did not silently disable anything. This was the one thing that genuinely needed re-proving, since
AnalysisLevel 10.0could have quietly narrowed the rule set. Probe undersrc/, withAnalysisLevelpinned:Every rule I verified at
0829da4still fires at54df2aa. (I count 6 distinct messages across 5 rule IDs against your 7 — different probe files, not a discrepancy.)CA1707scoping re-proven both ways under the pin:Underscore_Named_Method_ReturnsTrueundertests/→0 Warning(s), exit 0; the identical name undersrc/→error CA1707, exit 1. Clean solution build afterwards: 0/0.The five non-blocking items
dotnet format --verify-no-changesin the pre-PR block — done, and better than I asked. The prose explains why it is not redundant and warns against dropping the flag, which is the failure mode that would have quietly reintroduced the problem. My BOM proof is reproduced accurately.CS86xxblock — removed, and the replacement comment says the right thing:Nullablemakes the whole family warnings andTreatWarningsAsErrorsmakes all of them errors, so restating six would be a no-op that reads as exhaustive. That is precisely the objection..razor.csmitigation — in bothCONTRIBUTING.mdand ADR-0013, and correctly qualified in the ADR as "a convention, not an enforcement — nothing stops logic accumulating in the markup file". Honest.[tests/**/*.{cs,razor}]— widened, with the comment stating plainly that it has no effect today. Right call: the scoping is correct in intent even while inert.AnalysisLevelpinned to10.0— accepted rather than argued down, and the reasoning added to ADR-0013 is better than my one-line suggestion. The distinction drawn againstNU19xxis the correct one and worth quoting: a disclosed vulnerability is a signal worth interrupting a build for, a newly added style rule is churn. Critically, the cost is recorded rather than presented as a free win — "new analyser rules ... are simply not applied until somebody bumps the number, and nothing prompts them to". That is the real downside of pinning and it is now on the record.The unprompted
CA1707comment fix is the right instinct. The old wording claimed the rule "cannot be narrowed", which would have contradicted the corrected objection 3 in ADR-0012 within the same commit. Catching a consistency break between a config comment and an ADR is the kind of thing that normally goes unnoticed for a year.Remaining, all trivial and none blocking
CLAUDE.mdremains untouched, so a session loading only it still learns nothing aboutDirectory.Build.propsor the "never put<Nullable>in a.csproj" rule. The acceptance criterion namesCONTRIBUTING.mdand is met; this is a two-line follow-up wheneverCLAUDE.mdis next edited.Summary
Every claim in the response reproduced, including the one that contradicted me. The denylist finding is a real result and it improved the record rather than just defending it — the ADR now tells the next reader what to build, what it will cost, and the specific reason the obvious version does not work, which is worth more than either shipping the script or omitting the option. The acceptance criterion is still formally unmet, but it is now unmet with a measured, quantified justification and a named upgrade path with a trigger, which is the right resolution.
Good to merge.