docs: full documentation audit — accuracy fixes, XML docs, planned-package framing - #125
Merged
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c74805baf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
Summary
A full audit of the repository's user-facing documentation — README,
docs/**,CLAUDE.md, and public XML doc comments — checked against the actual source, with every defect found corrected. No product behavior changes.What was wrong
Examples that would not compile
README.mdcalled.AddLinks(), which does not exist anywhere in the builder API.README.md,docs/api.md, anddocs/serialization.mdreferenced aChatter.Rest.Hal.Extensionsnamespace. No such namespace exists — every extension class declaresnamespace Chatter.Rest.Hal.README.mdanddocs/usage.mdwere missingusing Chatter.Rest.Hal.Builders;.docs/usage.mdlooked up a link object by name against an example that only ever setTitle, so the documented lookup could never match.Documented behavior that contradicted the code
README.mddynamic example's "resulting JSON" predated the 2.0.0 curies change:curiesserializes as an array even for a single definition (_isArray = rel == CuriesLink), and the example'sea:adminrelation holds two link objects, not one. The block was regenerated from real serializer output.AddHalConvertersidempotency was described as a whole-batch bail-out in three documents. It is a per-converterAddIfMissingguard: a consumer with a subset pre-registered gets only the missing converters, and pre-registered converters keep their ownHalJsonOptions.docs/architecture.md's converter constructor table did not match the real constructors.ResourceConverter's read/write description was wrong indocs/architecture.mdanddocs/serialization.md.GetLinkOrDefault(SingleOrDefault, throws on duplicate relations) and the twoGetLinkObjectOrDefaultoverloads (by relation isFirstOrDefault; by name isSingleOrDefault) had their contracts undocumented or misstated. That asymmetry is now explicit.AddResource()vsAddResources()return-target semantics were conflated in both the stage XML docs anddocs/api.md.docs/development.mdclaimed "There is no global.json in this repository." There is one, pinning SDK 8.0.100 withrollForward: latestMinor. Target-framework claims were also wrong:Chatter.Rest.Hal.CodeGeneratorsisnetstandard2.0-only.Stale and dead content
docs/architecture.mdreferenced package version 1.1.0 in several places.docs/uri-templates/*—Chatter.Rest.UriTemplatesis an external NuGet dependency with no in-repo docs.Missing coverage
LinkCollectionBuildermerges repeatedAddLink/AddSelf/AddCuriesviaGetOrAddLink) and the curies array default were undocumented.LinkCollection.AddandEmbeddedResourceCollection.AddthrowArgumentException, while deserialization normalizes duplicates last-wins.HalResponseAttributeandHalResponseGenerator. Since the builder's whole discoverability story is IntelliSense on the next stage, this was the largest gap in the audit. The new comments follow the two already-documented sibling stage files.Framing
docs/aspnetcore/*describedChatter.Rest.Hal.AspNetCorein shipped present tense, but no such project exists and nothing is published. Both documents now carry a status banner marking them design specs for a planned package, and theCLAUDE.mddocumentation index says so too. The design content itself is unchanged; only the tense and the banner.Resource<T>is annotated as a planned core addition rather than deleted.Review remediation
Codex raised one P1 on the initial push, and it was correct: the release advertised new XML documentation comments, but neither packable csproj set
GenerateDocumentationFile, so no XML sidecar was produced or packed. Consumers of the published package would have gotten no IntelliSense from any of the 22 newly documented types — the documentation existed only in source.Fixed in
285bd84and10acd3a:src/Chatter.Rest.Hal/Chatter.Rest.Hal.csprojsetsGenerateDocumentationFile=true. A packed.nupkgwas inspected and contains bothlib/net8.0/Chatter.Rest.Hal.xmlandlib/netstandard2.0/Chatter.Rest.Hal.xml.</remarks>blocks in each ofIEmbeddedLinkObjectPropertiesSelectionStage.csandIResourceLinkObjectPropertiesSelectionStage.cs— the malformation had been causing the compiler to silently discard the surrounding comments — plus interface summaries andAsArray()documentation for both. Two unresolvableJsonNode.Deserializecrefs (ConverterHelpers.cs,ResourceConverter.cs) replaced withJsonSerializer.Deserialize{TValue}(JsonNode, JsonSerializerOptions). Two ambiguousState{T}crefs inResource.csclosed toState{T}(JsonSerializerOptions?), the overload that actually holds the Link-guard the surrounding prose describes.CHANGELOG.md[2.1.1]extended to state the packed sidecar and the doc repairs.The build now emits zero documentation warnings of any code (CS1570, CS1574, CS1591, CS0419 all clear). Beyond fixing the reported symptom, this closes the class: with generation enabled the compiler surfaces missing or malformed doc comments on every build, so "documented in source, invisible to consumers" cannot silently recur for this package.
Deliberately not applied to
Chatter.Rest.Hal.CodeGenerators. That package setsIncludeBuildOutput=falseand packs its assembly only intoanalyzers/dotnet/cs, which consumer code never references — an XML sidecar there would be unreachable by any consumer's IDE. Its consumer-facing surface is the emittedHalResponseAttribute.g.cs, andAttributeSource.csalready embeds doc comments in that emitted source, so[HalResponse]IntelliSense already works.No further version bump: 2.1.1 is unpublished until merge, so the sidecar ships with that release.
Versioning
Chatter.Rest.Hal2.1.0 → 2.1.1,Chatter.Rest.Hal.CodeGenerators0.4.0 → 0.4.1, with CHANGELOG entries.These are PATCH bumps for a change that alters no API and no behavior. They are required mechanically:
version-check.ymlfires on any non-markdown change under a package's src path, XML doc comments are.csedits, and tagshal/v2.1.0andcodegen/v0.4.0already match the prior versions, so the check would fail without a bump.The 2.1.1 bump additionally covers the packaging change above — the package now ships an XML documentation file it did not ship before.
Validation
dotnet build Chatter.Rest.Hal.sln -c Release— succeeded, 0 errors, 0 documentation warnings. The 3 remaining CS8604 warnings are pre-existing nullability warnings, untouched.dotnet test -c Release— 507/507 passed (456 HAL, 51 CodeGenerators).dotnet packoutput inspected to confirm the XML sidecar is present for both target frameworks.Follow-ups (not in this PR)
Chatter.Rest.Hal.CodeGenerators.csprojreferencesChatter.Rest.Halat 1.1.0. Left untouched because changing it is a build change, not a documentation fix, and warrants its own validation.Converters/LinkConverter.cs:122andtest/Chatter.Rest.Hal.Tests/Converters/ConverterWriteAndRegistrationCleanupTests.cs:188,208. Executable-code changes, outside a documentation PR.docs/backlog.md,docs/consistency-audit.md,docs/performance-todo.md,docs/client/,docs/mcp/,CONTRIBUTING.md,CONTEXT-MAP.md, and the per-projectCONTEXT.mdfiles were outside this audit's scope. Mostly internal working docs rather than the user-facing surface, but available as a follow-up pass.docs/development.md§8 claims netstandard2.0 nullable warnings originate in converter files. Plausible but unverified — a product-code claim rather than a documentation defect, so it was left alone.