chore: Unify and automate code style, formatting and member ordering across the repository (issue #21) - #22
Merged
Conversation
Groups the settings by concern, adds the missing [*] section, drops the UTF-8 BOM, and reverses the type-name rule: dotnet_style_predefined_type_for_* go to error, so `string` and `int` replace `String` and `Int32`. Also removes the IDE0290 suppression, which contradicted csharp_style_prefer_primary_constructors in the same file. The code catches up in the next two commits. Part of #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mechanical. Applied with:
dotnet format style DbConnectionPlus.slnx --diagnostics IDE0049
String becomes string, Int32 becomes int, Boolean becomes bool, and so on, in
declarations and in static member access alike. 7655 diagnostics across 207
files. No hand edits: the only non-rename change in the tree was the
.editorconfig commit before this one.
Cref references in XML documentation are untouched. IDE0049 does not rewrite
them, and <see cref="String" /> resolves the same either way.
This commit is listed in .git-blame-ignore-revs.
Part of #21
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`dotnet format style --diagnostics IDE0290`. Six sites. Carries one hand edit the fixer cannot do: it leaves the old `<summary>Initializes a new instance...</summary>` blocks behind after moving the parameter documentation to the type, and those 23 lines are removed here. Reproducing the commit therefore differs by exactly those lines. Part of #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
EnforceCodeStyleInBuild and TreatWarningsAsErrors move to the root Directory.Build.props, so tests/ and benchmarks/ are held to the same style as the libraries. The CA quality rules stay in src/ only - CA1707 alone fires 2100 times on the Method_ShouldDoSomething naming the test suite is built around. CSharpier becomes the formatter: pinned in .config/dotnet-tools.json, with .csharpierignore for the hand-maintained XML. IDE0055 goes off, because whitespace is now CSharpier's and the two disagree. The print width comes from max_line_length in .editorconfig, so it has one source of truth. Part of #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mechanical. Applied with:
dotnet csharpier format .
230 files. From here on nobody places a line break by hand: CSharpier reprints
the file and the result is the same whoever - or whatever - wrote the code.
Two things came through untouched, both worth knowing:
- The 280-line conversion table in ValueConverterTests marked `@formatter:off`.
Every row already fits inside 120 characters, so CSharpier leaves it exactly
as it is and needs no csharpier-ignore directives.
- Comment text. CSharpier does not rewrap the inside of a comment, so the XML
documentation across the shipping libraries is unchanged.
Verified after the reformat: Release build clean, 0 warnings, and the full test
suite green - 11005 passed, 0 failed, integration tests included.
This commit is listed in .git-blame-ignore-revs.
Part of #21
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NewStyleCop.Analyzers reports a wrong order; it cannot fix one, because its ElementOrderCodeFixProvider is [NoCodeFix] and never registered. ReSharper does the fixing, through the file layout in DbConnectionPlus.slnx.DotSettings and a cleanup profile that reorders members and nothing else. So the order is defined twice and the two have to stay in step: stylecop.json is what is checked, the .DotSettings layout is what is applied. Every StyleCop category is switched off in .editorconfig and only the ordering rules plus SA1309 switched back on, so adding a StyleCop rule here is deliberate. Part of #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mechanical. Applied with:
dotnet jb cleanupcode DbConnectionPlus.slnx --profile=ReorderMembers
dotnet csharpier format .
133 files. Members now follow SA1201's order - fields, constructors,
finalizers, delegates, events, enums, interfaces, properties, indexers,
conversions, operators, methods, nested types - with constants ahead of fields,
public ahead of private, static ahead of instance, readonly ahead of mutable,
and alphabetical inside each group.
The visible change is that fields move to the top of the type. This codebase
kept them at the bottom; StyleCop puts them first.
CSharpier runs after the reorder, because moving a member re-indents it and
ReSharper indents to its own settings rather than CSharpier's. That is the
order scripts/tidy-cs.ps1 uses too.
On the one real risk - reordering fields changes the order their initializers
run in, which can change behaviour. It does not here: a scan of every field
initializer in the repository finds none that reads another field of the same
type, so no initializer depends on the order. The full suite is green as well:
11005 passed, 0 failed, integration tests against all five databases included.
This commit is listed in .git-blame-ignore-revs.
Part of #21
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Explicit interface implementations get their own file layout entry per kind
(property, indexer, method), matching ImplementsInterface AND Access Is=Private.
Without the access test the entry also catches implicit implementations and drags
Equals(T) away from Equals(object). Events must NOT have one: StyleCop counts an
explicit event as private, and giving it an entry breaks the build.
Four gates added:
IDE0005 unused using. Needs GenerateDocumentationFile on every project, or
it is never reported at build time (dotnet/roslyn#41640).
RCS1250 target-typed new, where IDE0090 cannot reach - return, argument and
assignment positions.
SA1208/9/10/11/17 using order. These check rather than fix: CSharpier already
sorts usings. SA1216 is excluded because CSharpier contradicts it.
CSharpier.MsBuild an unformatted file becomes a build error, in check mode so
the build never rewrites sources.
The code catches up in the next commit.
Part of #21
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mostly tool output:
20 unused using directives removed dotnet format style, IDE0005
14 object creations to target-typed new dotnet format analyzers, RCS1250
7 files reordered jb cleanupcode, after the layout gained
the explicit-interface entries
By hand, where no tool reaches:
IDE0049 misses nint/nuint, never looks inside nameof(), and never sees
tests/package-consumption/ because it is not in the solution. Converted there.
MySqlEntityManipulator was the last adapter on a classic constructor, hidden
behind a stale #pragma warning disable IDE0290.
Two global using Xunit lines the SDK already generates. Redundant rather than
unused, so IDE0005 correctly stays quiet.
Two Query test lambdas keep an explicit .ToList(): the lambda is an Action, so
its body must be a statement and a collection expression is not one (CS0201).
Part of #21
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
scripts/format-cs.ps1 becomes scripts/tidy-cs.ps1, in three scopes: (default) ~1s CSharpier, on the files git reports as changed -Scope style ~15s + the Roslyn code-style fixers -Scope all ~3min + member reordering, whole solution -Check reports instead of fixing. With -Scope all it still WRITES, and that is not a shortcut: cleanupcode re-indents raw string literals and CSharpier puts them back, so neither is idempotent alone and asking either in isolation always answers yes. The pair is, so it tidies for real and compares the tree before against after. Both editor hooks run the default scope; preflight.ps1 runs -Scope all before the build; CI's lint job runs -Check -Scope all, so CI cannot disagree with the local tooling. CI also fails if a .git-blame-ignore-revs entry stops resolving - git does not warn about one it cannot resolve, it silently skips it. Every dotnet call goes through one helper that relaxes $ErrorActionPreference: at Stop, PowerShell turns anything a native tool writes to stderr into a terminating error, and ReSharper writes a harmless warning most runs. Part of #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AGENTS.md is the canonical guidance for both agent integrations and has a 200
line instruction budget, so the detail moves to
.agents/references/code-style.md and AGENTS.md keeps the rules and the traps.
What an author has to know, because no tool enforces it:
- A primary constructor parameter goes into a private readonly backing field
and is read through this.field. A captured parameter compiles to a field
with no readonly, so using it directly drops that guarantee silently.
- IDE0049 misses nint/nuint, nameof() and tests/package-consumption/.
- Same-named overloads have no defined order; ReSharper's sort is stable and
leaves them where it found them. Both orders are correct.
- cleanupcode re-indents raw string literals and CSharpier puts them back.
CONTRIBUTING.md gains a clone setup section, and Conventional Branch replaces
the two-prefix branch rule that had no room for a change like this one.
Part of #21
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Last commit on the branch by necessity: it names commits by SHA, so they have to exist first. Part of #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
What does this change?
Closes #21.
Unifies and automates code style, formatting and member ordering across
src/,tests/andbenchmarks/. Three concerns, three tools, no overlap between them:.editorconfig,.csharpierignore.editorconfigDbConnectionPlus.slnx.DotSettings,stylecop.jsonAll three are build errors now, in
tests/andbenchmarks/as much as insrc/— style and orderingthrough the analyzers, formatting through
CSharpier.MsBuildin check mode, so the build never rewritesyour files. One entry point applies them,
scripts/tidy-cs.ps1in three scopes, and CI runs that samescript, so CI cannot disagree with the local tooling.
preflight.ps1and both agent hooks call it too.12 commits, 292 files. Almost all of it is machine output and changes no behaviour, with one
deliberate exception: primary constructor parameters are held in
private readonlybacking fields. Acaptured parameter compiles to a field with no
readonly, so using one directly loses the compiler'sguarantee that the value cannot be reassigned.
Also closes gaps nothing was watching: 20 unused using directives (
IDE0005was never reported at buildtime, because that needs
GenerateDocumentationFile), 14 object creations that should be target-typed(
RCS1250, whereIDE0090cannot reach), and the order of using directives (SA1208/1209/1210/1211/1217)..git-blame-ignore-revsnames five commits by SHA. "Rebase and merge" and "Squash and merge" rewritethose SHAs, and git never warns about one it cannot resolve — it skips the entry, and blame quietly
goes back to pointing at CSharpier instead of at whoever wrote the line. The lint job now fails if any
entry stops resolving, but that catches the damage rather than preventing it.
Reviewing this
Run
git config blame.ignoreRevsFile .git-blame-ignore-revsonce, and the mechanical commits drop outof blame.
The commits come in prepare / apply pairs: the odd one sets a rule, the next satisfies it with one
tool. Read them one at a time, and re-run the tool from the prepare commit to check the apply commit:
.editorconfigIDE0049)IDE0290)All five were re-run from their parent commit and the differences above are the measured result, not an
estimate.
The member reorder does not reproduce exactly, by design: same-named overloads tie on every sort key
and ReSharper's sort is stable, so it leaves them where it found them. Both orders are correct. Its check
is instead that the tip is a fixed point —
tidy-cs.ps1 -Scope all -Checkexits 0.The last pair is a mixed commit, so re-running its tools reproduces only part of it. The other 32
files are hand edits that no tool in the pipeline can make: the keyword conversions under
tests/package-consumption/and insidenameof(...), theMySqlEntityManipulatorconversion that astale pragma had been hiding, two
global usinglines the SDK already generates, and a set of commentsand doc-wording fixes.
Checklist
dotnet build DbConnectionPlus.slnx -c Releasesucceeds with zero warnings.pwsh -File scripts/preflight.ps1passes.tests pass unchanged, which is the point.
IL2xxx/IL3050diagnostics. No reflection path changed.PublicAPI.Unshipped.txt. — No public API change; noPublicAPI.*.txtfile is touched by this branch.README.mdupdated for public API changes. — N/A, see above.CHANGELOG.mdupdated and the version bumped. — N/A: no user-visible change and nothingrelease-bound, so no entry and no version bump.
chore/issue-21-code-style-toolchain.