fix(docs): stop MaxMonoid shipping with no documentation at all - #358
Merged
Conversation
An unclosed <para> in MaxMonoid<T>'s <remarks> made the doc comment invalid XML. The doc writer does not truncate a malformed comment, it drops the whole member element, so `T:Celerity.Collections.MaxMonoid`1` was absent from the shipped Celerity.xml entirely: no summary, no typeparam, and none of the floating-point caveat (a stored -inf aggregates to T.MinValue; NaN resolves by operand position). The issue reported the caveat as at risk; it was in fact already gone, along with the rest of the type's docs. Closing the tag restores the entry. Two gates keep it from recurring: - CS1570 joins CS1591 in WarningsAsErrors across all seven shipping packages, so a doc comment must now be present *and* parse. This turns the two warnings the defect emitted into build errors, verified by rebuilding against the original comment. - XmlDocumentationTests asserts, against the artifact that actually ships, that each package's .xml parses and carries an entry for every public type its assembly declares. Both new tests fail on the pre-fix comment. CONTRIBUTING.md, CLAUDE.md and the ROADMAP doc-coverage line are updated to describe both gates rather than CS1591 alone. Closes #356 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
Coverage
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes malformed MaxMonoid<T> XML documentation and prevents similar packaging regressions.
Changes:
- Closes the missing
<para>tag. - Promotes CS1570 to an error across seven packages.
- Adds XML documentation artifact tests and updates contributor documentation.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Celerity/Collections/MaxMonoid.cs |
Repairs malformed XML documentation. |
src/Celerity/Celerity.csproj |
Enforces CS1570. |
src/Celerity.Hashing/Celerity.Hashing.csproj |
Enforces CS1570. |
src/Celerity.Primitives/Celerity.Primitives.csproj |
Enforces CS1570. |
src/Celerity.Sorting/Celerity.Sorting.csproj |
Enforces CS1570. |
src/Celerity.Ring/Celerity.Ring.csproj |
Enforces CS1570. |
src/Celerity.Sentinel/Celerity.Sentinel.csproj |
Enforces CS1570. |
src/Celerity.Cardinality/Celerity.Cardinality.csproj |
Enforces CS1570. |
src/Celerity.Tests/Packaging/XmlDocumentationTests.cs |
Verifies generated documentation artifacts. |
CONTRIBUTING.md |
Documents XML-comment requirements. |
CLAUDE.md |
Updates agent guidance. |
ROADMAP.md |
Records the expanded documentation gate. |
CHANGELOG.md |
Adds release notes for the fix and safeguards. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The separate XmlDocumentationTests bullet named a test class rather than an observable change, against the "short and user-facing, one tight entry per change" rule in CONTRIBUTING.md. The prevention outcome now rides on the CS1570 entry as a clause; the test detail lives in the PR body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CHANGELOG.md:9
- This overstates the regression coverage:
XmlDocumentationTestsanchors only Celerity, Hashing, Primitives, and Sorting; its own comment says Ring, Sentinel, and Cardinality rely solely on the compiler gate (XmlDocumentationTests.cs:27-40). Remove the claim that every package's XML is asserted, or qualify it as the four reachable packages.
- **CS1570 (badly formed XML in a doc comment) is now a build error** in all seven shipping packages, alongside the existing CS1591 gate, and each package's shipped `.xml` is asserted to carry an entry for every public type. A malformed comment is not truncated by the doc writer — the whole member is dropped — so a warning was too weak a signal for something that silently empties a type's documentation. Closes [#356](https://github.com/marius-bughiu/Celerity/issues/356).
The folded entry read as though the shipped .xml of all seven packages is asserted. It is the CS1570 gate that covers seven; XmlDocumentationTests reaches the four packages Celerity.Tests references, and the three showcase packages rely on the compiler gate alone. Reworded so the scope claim belongs to the gate, not the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Copilot's round-2 review suppressed one comment on
That overclaim was introduced by my own fold in de4ebb6 — "each package's shipped
I did not extend On the benchmark note in the PR body: I wrote that |
Benchmarks2 regressions Highlights
Collections (646)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( Observed spread of this run — |Δ| across all 757 paired rows: p50 0.4%, p90 4.8%, p95 8.7% (nearest-rank). A pull request usually touches a handful of these, so this is mostly the runner's own drift between the two slices — but a change to a shared primitive moves many rows at once and would raise these figures itself, so read it as the run's spread rather than as a floor the PR cannot have caused. A flag that does not stand out against it is worth re-running before acting on. |
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
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.
Closes #356.
The defect was worse than filed
The issue reported that
MaxMonoid<T>'s unclosed<para>put its remarks at risk. Checking the generatedCelerity.xmlonmain, the damage is total — the member element is absent entirely:There is no
T:Celerity.Collections.MaxMonoid1entry. The doc writer does not truncate a malformed comment, it drops the whole member — so the type shipped with **no summary, notypeparam, and no remarks**, and an IDE tooltip onMaxMonoidshows nothing. The floating-point caveat (a stored-∞aggregates toT.MinValue;NaN` resolves by operand position) was already gone, not merely at risk.What changed
Fix —
src/Celerity/Collections/MaxMonoid.cs: one</para>. TheT:entry is back in the shipped XML with its summary,typeparamand both<para>blocks. No behavioural change.Compiler gate — CS1570 joins CS1591 in
<WarningsAsErrors>across all seven shipping packages (Celerity,Celerity.Hashing,Celerity.Primitives,Celerity.Sorting,Celerity.Ring,Celerity.Sentinel,Celerity.Cardinality). A doc comment must now be present and parse. Verified by rebuilding against the original comment: the two warnings becomeRegression test —
src/Celerity.Tests/Packaging/XmlDocumentationTests.csasserts against the artifact that actually ships, not the source:DocumentationFile_ShouldParseAsWellFormedXml_...— each package's.xmlloads.DocumentationFile_ShouldDescribeEveryPublicType_...— every public type the assembly declares (forwarded types are filtered out, since they are documented where they are declared) has a<member name="T:…">entry.MaxMonoid_ShouldShipItsSummaryAndRemarks_...— pins the specific entry, including theNaNcaveat.Both of the latter two fail on the pre-fix comment, with the assembly-wide one naming the culprit:
It covers the four packages this test project reaches; the three showcase packages are not referenced from here and rely on the compiler gate.
Docs —
CONTRIBUTING.md,CLAUDE.mdand theROADMAP.mddoc-coverage line each described the CS1591 gate alone and are updated to cover both, with the reason the second one is needed.CHANGELOG.mdgets### Addedbullets for the gate and the tests, and a### Fixedbullet for the defect.Parity checklist
XmlDocumentationTests(9 cases)Program.csscripts/benchmark_relevant_changes.jsonly proves comment-only.csedits inert, and this diff touches.csprojfiles, which it treats as significant by designweb/…)COLLECTIONSentry or ship carddocs/api/collections.md.xmlwas wrong.README.mdlikewise needs no changeCHANGELOG.mdROADMAP.mdTest plan
dotnet build --no-incremental— 0 warnings, 0 errors (Debug and Release).dotnet teston net8.0 / net9.0 / net10.0 — 5658 passed, 0 failed each.Celerity.Ring.Tests46,Celerity.Sentinel.Tests30,Celerity.Cardinality.Tests37 — all pass, confirming the CS1570 promotion breaks nothing in those packages.MaxMonoid.csand rebuilt: CS1570 fires as an error, and the two new tests fail — the gate and the regression test both bite.node scripts/check_doc_anchors.js— 585 links across 23 markdown files resolve.🤖 Generated with Claude Code