chore(style): enforce file-scoped namespaces and PascalCase public members at build time - #703
Conversation
…mbers at build time Adds the two style rules the repo had been relying on habit for, and wires EnforceCodeStyleInBuild so they run in `dotnet build` (and therefore CI) rather than only as editor squiggles. IDE0161 (file-scoped namespaces) is repo-wide; CA1707 (no underscores in public member names) is scoped to Daqifi.Core, the only project that publishes a public surface, and left at its default public-only api_surface so it does not fight the repo's own _camelCase private-field convention. CA1707's only two hits were IntelHexParser's shipped constants, renamed to DefaultBeginProtectedAddress / DefaultEndProtectedAddress with [Obsolete] forwarders that keep the old spellings compiling, per ADR 0002. Also drops the ScpiMessageProducer.TurnDeviceOff line from DEVICE_INTERFACES.md - that member has never existed, so the snippet did not compile. Refs #484
…coped Mechanical, produced by `dotnet format style Daqifi.Core.sln --diagnostics IDE0161`. Reviewable as `git diff -w`: ignoring whitespace the change is 100 files, 201 insertions and 300 deletions - the namespace line, its opening brace, and its closing brace, and nothing else. The rest of each file's diff is the one-level dedent. Refs #484
…rwarders The analyzers are the real guard; what they cannot guard is their own wiring. Dropping EnforceCodeStyleInBuild, or setting either severity to none, leaves the build green with the style unpoliced - so CodeStyleEnforcementTests asserts both. Each was mutation-tested: build green, test red. IntelHexParserTests gains one case, that the two shipped SCREAMING_SNAKE names are still public and still carry [Obsolete]. Two further candidates - the forwarders equalling their replacements, and the constants keeping their shipped literals - were written, mutation-tested, and dropped: PublicAPI.Shipped.txt records a public const by value, so both mutations already fail RS0016/RS0017 rather than passing a green build. Also documents both rules in CONTRIBUTING.md next to the existing CA2007 and PublicAPI sections. Refs #484
|
/agentic_review |
PR Summary by QodoEnforce namespace and public member naming during builds
AI Description
Diagram
High-Level Assessment
Files changed (106)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full Previous reviewsReview updated until commit 4f71f20 Results up to commit f767eb6
|
#702 (TimeProvider seam) landed on main and touched eight files this branch had converted to file-scoped namespaces, so every conflict was #702's content against this branch's dedent of the same lines. Resolved by taking origin/main's version of all eight verbatim and re-running `dotnet format style --diagnostics IDE0161` over the result, rather than by hand- merging: this branch's only change to those files was the namespace conversion (verified with `git diff -w` against the merge base - not one substantive line), so re-deriving it mechanically cannot drop any of #702's work. Checked after the merge: `git diff -w` against origin/main shows no changed line outside .editorconfig, Directory.Build.props, CONTRIBUTING.md, DEVICE_INTERFACES.md, IntelHexParser.cs, PublicAPI.Unshipped.txt and the two test files that is anything other than a namespace or brace line.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 4f71f20 |
…e real handshake Two review findings on the ServerInstructions change: - The "retrieve before you stream" rule told every client that a live stream collapses the SD buffer and empties later downloads. That is firmware #703, which firmware 3.7.3 fixed. v3.7.2 is still the latest stable release, so the advice still matters, but stated unconditionally it is wrong for any device on the fix. The handshake rule, the download_sd_file description and the README now say "on firmware below 3.7.3", and point at the firmwareVersion discover_devices already returns. The recovery step now says "power-cycled" rather than "reconnected", which is what the firmware issue and SdCardEmptyTransferException both say clears it. - The instruction tests read ServerOptions.Instructions directly, so they stayed green with the o.ServerInstructions assignment in Program.cs deleted. ServerInstructionsHandshakeTests now starts the real server over stdio with the SDK's own client and reads `instructions` off the initialize result, with and without --read-only. Deleting the assignment turns both red on net9.0 and net10.0. The direct tests they replace are removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
What was wrong
Nothing checked code style, in a repo that otherwise treats every warning as an error. Namespace
style had split roughly down the middle — about 200 files file-scoped against 142 block-scoped —
because a new file copied whichever style the file next to it happened to use, and nothing ever
said which was right. And two constants had shipped on the public NuGet package spelled
DEFAULT_BEGIN_PROTECTED_ADDRESS/DEFAULT_END_PROTECTED_ADDRESS, a style that appears nowhereelse on the public surface and that gets more expensive to correct with every release.
The docs also told readers to call
ScpiMessageProducer.TurnDeviceOff. That member has neverexisted — only
TurnDeviceOn— so anyone copying that snippet got a compile error.How it was fixed
Two rules now fail the build rather than waiting for someone to catch them in review: IDE0161
(file-scoped namespaces, repo-wide) and CA1707 (no underscores in public member names, scoped
to
Daqifi.Core).EnforceCodeStyleInBuildinDirectory.Build.propsis what makes them runoutside an editor. The two constants are renamed to
DefaultBeginProtectedAddress/DefaultEndProtectedAddress, with[Obsolete]forwarders so source written against the releasedpackage still compiles — the source compatibility ADR 0002 promises. The 142 block-scoped
namespaces are converted, and the bad doc line is gone.
Three things worth pushing back on:
api_surface. Widening it toallsoundsstricter and is wrong here: it also reports private static fields written in this repo's own
_camelCaseconvention (SdCardFileParserFactory._supportedExtensions). Eight privateSCREAMING_SNAKEconstants therefore survive. They cost nothing outside the assembly, andrenaming them under a rule that would not police them afterwards is churn, not enforcement.
9b196ae), produced bydotnet format style --diagnostics IDE0161, and the way to read it isgit diff -w: ignoringwhitespace it is 201 insertions and 300 deletions — the namespace line, its opening brace and
its closing brace, per file, and nothing else. I checked that mechanically: with
-wthere isnot a single changed line that is not one of those three.
failing this repo's bar that a guard must be able to fail on a green build:
PublicAPI.Shipped.txtrecords a public const by value, so a forwarder drifting from itsreplacement, or a literal changing, already fails RS0016/RS0017.
[Obsolete]is not recordedthere, so that is the one thing left worth asserting.
Verified
dotnet test Daqifi.Core.sln -warnaserrorgreen on both frameworks: 4139 passed / 3 skipped(net9.0 and net10.0), plus 217 Daqifi.Mcp tests on each.
public const int SOME_VALUEinDaqifi.Corefails CA1707; a new block-scoped namespace in the test project fails IDE0161.
EnforceCodeStyleInBuild, setting IDE0161 tonone, setting CA1707 tonone, and dropping[Obsolete]from a forwarder each leave0 Error(s)and turn a test red.DEFAULT_*names compiles against this branch, with CS0618 naming the replacement./dev/cu.usbmodem1101: connect → status (analogIn=16, digital=16) →AI0/AI1 → 100 Hz → 3 s CSV capture (236 rows) → clean disconnect, exit 0. Non-destructive. The
~79% effective rate is the known fw 3.7.2 clock mismatch, not a change from this PR.
closes #484