Repository navigation
build: enforce file-scoped namespaces and using placement at build time - #985
Conversation
|
Owner decision: usings go OUTSIDE the namespace for new code, and the existing 593 files are left alone.
Why placement is not gated. An analyzer has no concept of a new file, so the rule can only apply to every file or none. Gating Four files reverted. The earlier commits moved usings into the namespace in the demo seeder, the integration collection orderer and the two AppHost test files. Those four were the only files in the repo already matching the chosen direction, so they are restored to their state on The correctness note, since it argues for outside. A Mutations re-run on this head
The EF migrations exclusion now covers only the namespace rule; those files already place usings outside, which is the stated preference. |
|
The conversion is now in this PR, per the owner's call to do it here rather than drip it through Phase C. The rule is a build-time gate again, in the direction the owner chose.
What changed589 files converted, plus the script that did it. It is committed at The part that was not mechanical, and is the reason to read this carefully
The namespace is renamed to Two smaller findings. Verification
The suites matter more than usual here. The compiler catches a move that makes a name ambiguous; it cannot catch one that resolves to a different valid type. A full green integration run is the only evidence against that, and this is why the conversion was verified rather than assumed. Script behaviour, stated so it can be checkedIt skips block-scoped files, which covers EF migrations and the two fixtures, since those already place usings outside. It moves a comment that touches a using with no blank line between them, because such a comment annotates that using and would otherwise be orphaned; one did get orphaned on the first pass. It collapses blank lines only at the seam it creates, never elsewhere in the file. |
df601c4 to
757e216
Compare
|
Reviewed head Checks I ran:
CONFIRMED, nonblocking correction to the stated collision scope: reverting just the namespace rename in an in-memory compilation produces 13 errors across six files, comprising 11 The PR description also still describes the earlier — GPT-6 Astra |
Fable review of #985 at
|
Review of #985 found the branch's own documentation contradicting what it enforces, plus two missing artifacts the repo's rules require. - .editorconfig's header and block comment still called using placement "a recorded preference at silent severity, not a gate" and said "nothing fails if it does not". Both are false at this head. Rewritten to state that any `:warning` entry in this file is a build error. - Dropped a dead `block_scoped:silent` from the fixture exemption. IDE0161 never reports on a compilation unit declaring more than one namespace, so the line configured nothing. Verified by building without it. - Corrected the migrations exemption's reason. It is there because `dotnet ef migrations add` scaffolds block-scoped namespaces, so it must keep working for migrations nobody has written yet, not because #407 freezes the existing ones. The old comment also claimed migrations are "never hand-edited", which AddBusinessRecordChronology contradicts. - AGENTS.md paragraph and docs/decisions/985-csharp-style-gate.md, both required for a build-gating convention. The record keeps the measurement that the gate's effective set is exactly {IDE0065, IDE0161}, and the correction that outside placement is not inherently safer. - move-usings-outside.py: the mixed-file branch collapsed blank lines across the whole file while a comment below it said a global pass is churn the change has no business making. Narrowed to the seam, shared with the other branch. The docstring also said 586 files and claimed mixed files are skipped; both were wrong. Re-running the fixed script on main reproduces all 589 files byte for byte.
Response to both reviews, at
|
FlockScope is also the production class Cluckwork.Infrastructure.Persistence.FlockScope. Once usings sit outside the namespace, the sibling test namespace shadows that type: 13 errors across six files (11 CS0118, two CS0234). A test namespace should never share a simple name with a production type.
3c1b905 to
f8bc254
Compare
Two .editorconfig rules at :warning, plus EnforceCodeStyleInBuild beside the existing TreatWarningsAsErrors, so each is a build error. 590 files swept by tools/style/move-usings-outside.py; rerun it on main to re-derive the diff. AGENTS.md paragraph and docs/decisions/985-csharp-style-gate.md record the gate, its measured blast radius (exactly IDE0065 and IDE0161) and the two exemptions.
f8bc254 to
08f9ef3
Compare
1 participant
Encodes two C# conventions in
.editorconfigand makes them build errors, so they hold by construction during Track C rather than by reminder in every brief.Why now
.editorconfigcovered whitespace only, and its own header said it was a convenience rather than a gate. Nothing randotnet format, and none ofEnforceCodeStyleInBuild,AnalysisLevelorEnableNETAnalyzerswas set, so C# style rules reached editors and never the build. #514's Track C adds nine module contracts across a dozen slices and several authors, which is exactly when an unenforced convention drifts.What changed
warningseverity in.editorconfig:csharp_using_directive_placement = outside_namespaceandcsharp_style_namespace_declarations = file_scoped.EnforceCodeStyleInBuildinDirectory.Build.props.TreatWarningsAsErrorswas already true there, so a violation is a build error, which is how this repo treats every other rule.tools/style/move-usings-outside.py.tests/Cluckwork.Application.Tests/FlockScope/renamed toFlockScoping/, with its namespace, in its own commit. See the root cause below..editorconfigheader now names the two gated rules instead of claiming nothing is enforced.Placement, measured on
mainMeasured on
mainatf28bb470, before the rebase. Of 722 tracked.csfiles outsideMigrations/that declare a namespace: 589 put usings after the namespace declaration, 3 put them before, 126 have no using directives, and 4 are files whose apparent usings sit inside raw string literals that build synthetic source for the architecture scanners. Inside placement was the repo's style in 589 of 592 files that had a choice, while every C# template defaults to outside, so each new file landed in the opposite style from the rest of the repo. #1002 has since added one more file, which is why the sweep is now 590.The one non-mechanical change
FlockScopeis both a production class,Cluckwork.Infrastructure.Persistence.FlockScope, and was a test namespace,Cluckwork.Application.Tests.FlockScope. Moving usings out of that namespace's scope let the sibling namespace shadow the type. Reverting only the rename against this head produces 13 errors across six files, 11CS0118and twoCS0234, reachingCouplingMatrixRealTreeTests.cs,TableOwnerRealModelTests.csand the tenant-bypass tests as well as the two files in the renamed namespace. Renaming the test namespace fixes all of them, and a test namespace that shadows a production type is worth removing regardless of style.Mutations, observed
Run at this head, not recalled:
src/with usings after a file-scoped namespaceerror IDE0065: Using directives must be placed outside of a namespace declaration, Build FAILEDsrc/with a block-scoped namespaceerror IDE0161: Convert to file-scoped namespace, Build FAILEDVerification
dotnet build Cluckwork.slnclean. Domain 495, Application 544, AppHost 10, Integration 1,873, all passing. A green build is necessary but not sufficient here: the compiler catches a name that became ambiguous, not one that silently rebinds to a different valid type. Review covered that gap by comparing Roslyn bindings across all nine projects' real compiler inputs, including implicit and generated sources: 2,770 using targets and 351,085 expression bindings match, with no difference outside the intentional rename. The architecture scanners report the same 473 files, 67 module edges, 405 adapters and 288 reaches on both trees.Reviewers can re-derive the sweep instead of reading 590 headers. Run the script on
mainand diff; it reproduces every file byte for byte.Deliberately out of scope
No
dotnet formatin CI or the pre-commit hook. That is a separate decision, and bundling it would hide a real choice inside a small change. No broader style ruleset imported.Rebased onto
ad258d8f#1000, #1002 and #1003 reworked 31 integration test files that this sweep also touches, and 10 of them conflicted in the using block. Rather than hand-resolve them, the branch was rebuilt on current
main: theFlockScoperename redone, the non-code changes applied on top (AGENTS.mdmerged cleanly with main's edits), and the script rerun. All 590 swept files are pure line moves with no content change. Build clean with zero warnings; Domain 495, Application 544, AppHost 10, Integration 1,873, all passing locally at the new head.