Skip to content

README: correct three claims that drifted from the code - #2

Open
dmccoystephenson wants to merge 2 commits into
mainfrom
feature/readme-accuracy-sweep
Open

dmccoystephenson wants to merge 2 commits into
mainfrom
feature/readme-accuracy-sweep

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

A Stage A documentation sweep was run. Every claim in README.md was checked against trace_client.hpp, Makefile, CMakeLists.txt and .github/workflows/build.yml. Three README claims were found that no longer matched the code, and they are corrected here. No code was changed.

  • Blank report names. enqueue returns early on a blank name (trace_client.hpp:900) and nothing is logged. The promise table said drops are passed to the Logger. The "Fits the server's limits" row now says a blank-named report is ignored without a Logger mention. This behaviour is already covered by reportIgnoresABlankName.
  • Emscripten reason. The constructor checks the environment, config and key before the platform (trace_client.hpp:790-797), so "unavailable" is the reason under Emscripten only when none of those opt-outs applies first. The README said it was always "unavailable".
  • MSVC build instructions. cmake --build build builds the Debug configuration with multi-config generators, but the README pointed at build/Release/test_trace_client. --config Release was added, which matches the windows CI job.

Not changed here

  • The README says TRACE_CLIENT_CURL overrides the curl executable. On Windows, findCurl prefers System32\curl.exe regardless of that macro, so the override has no effect there. Whether the code or the docs should change is the owner's call, so it is tracked in TRACE_CLIENT_CURL override is ignored on Windows when System32\curl.exe exists #1 rather than decided in a docs PR.
  • The rest of the sweep found no drift: the numbers in the promise table (256, 5 s, 255 bytes, 32 tags), the opt-out values and precedence, the disabledReason() strings, the curl command line and proto line, the wire-format headers, the User-Agent version (the same 0.1.0 in all three places), the Makefile targets, and the CI matrix description.

Test plan

  • README-only diff (git diff --stat origin/main: 1 file). No anchor-relevant files (trace_client.hpp, test/, Makefile, CMakeLists.txt) were touched, so the local make test anchor is recorded as UNVERIFIED-not-applicable.
  • CI Build is green on the PR head.

No tracking issue: this is a gap found during triage (Stage A sweep).

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

dmccoystephenson and others added 2 commits September 25, 2026 21:11
- A blank report name is ignored without a Logger mention; the promise
  table said every drop reaches the Logger.
- Under Emscripten the reason is "unavailable" only when no earlier
  opt-out (environment, config, no key) applied.
- The MSVC instructions built Debug but pointed at build/Release; add
  --config Release, as CI does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric (head d5fc0b0):

  • Scope: PASS. Only README.md is modified, and every hunk corrects one of the three drift points named in the PR body.
  • Tests-new: PASS (n/a). No new public method or function.
  • Tests-fix: PASS (n/a). This is not a bug fix, and no production code changed. The blank-name behaviour now documented is already asserted by reportIgnoresABlankName (test/test_trace_client.cpp:422).
  • Sibling structure: PASS (n/a). No new files.
  • Sibling renames: PASS (n/a). No renames.
  • Docs: PASS. Each corrected claim was re-checked against source:
    • README.md:35 — blank name: enqueue returns on detail::isBlank(name) before any log call (trace_client.hpp:900).
    • README.md:113-117 — Emscripten: the constructor tests environmentOptsOut(), !enabled and isBlank(key) before start() sets REASON_UNAVAILABLE (trace_client.hpp:790-799, :895).
    • README.md:157-159 — MSVC: the windows job in .github/workflows/build.yml runs cmake --build build --config Release and then build\Release\test_trace_client.exe.
  • Issue resolution: PASS (n/a). There is no Closes reference. The one code-vs-docs question found (TRACE_CLIENT_CURL on Windows) was filed as TRACE_CLIENT_CURL override is ignored on Windows when System32\curl.exe exists #1 and deliberately not decided here.
  • CI: PASS. All 11 Build jobs are green on the head SHA, including windows, macos and both linux-sanitizers legs.

Repo-specific items:

  • Registered, C++11, No new dependency, Never throws, Curl argv fixed, Sanitizers: n/a. The header, tests and build files are untouched. CI is green regardless.
  • Version triple: PASS. It is unchanged (0.1.0 in the header comment, in TRACE_CLIENT_VERSION and in the README User-Agent).
  • Local anchor: UNVERIFIED-not-applicable. The diff is README-only.

Finding fixed during review: the Emscripten edit left one line far wider than the rest of the wrapped paragraph. It was rewrapped in d5fc0b0.

Judgment call left for the reviewer: README.md:32 ("Never throws") still says "Drops are passed to the optional Logger". The blank-name exception is stated in the next row rather than in that sentence. That was judged clearer than qualifying the general promise, but it could be moved.

No do-not-auto-merge paths are touched. The PR is left open for human review and merge, because this dispatch is not authorized to merge.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant