Skip to content

[infra] Add a docs-only review guide - #971

Merged
wenjin272 merged 1 commit into
apache:mainfrom
weiqingy:894-2b-docs-guide
Aug 10, 2026
Merged

[infra] Add a docs-only review guide#971
wenjin272 merged 1 commit into
apache:mainfrom
weiqingy:894-2b-docs-guide

Conversation

@weiqingy

@weiqingy weiqingy commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Linked issue: #894

Purpose of change

Fills the docs-only row that #911 left as planned, the third of the four per-change-type guides. #957 filled the Python-Java bridge row the same way, and #961 is the api/ row.

Runtime flow

No executable path changes. The flow this PR completes is the routing one that #911 introduced.

A reviewer opens a documentation PR, reads the Change-Type Review Guides table in code_review.md, and follows the row matching the change. Before this PR the docs-only row terminated at the word planned, so the reviewer fell back to the general passes, which are written for code. After it, the row resolves to review-guides/docs-only.md, which narrows the passes to this change type and then hands back to the general ones.

Key decisions

The Validation section leads with a negative fact rather than a command list, because for this change type that is the useful content. A change under docs/content runs the full CI matrix and passes without any job reading a changed line. ci.yml has no path filter so every job runs, ruff runs with python/ as its root, and the site build lives in docs.yml, triggered on a schedule and by manual dispatch rather than on pull requests. There is no markdown linter and no link checker in the repo. A guide that listed commands here would be inventing them.

No local site-build command is published. tools/docs.sh mutates global git config with a container-only path, fetches a Linux x86-64 Hugo binary, and needs the theme submodule initialized, so it is not something a reviewer can be told to run.

The checklist is five bullets rather than the sibling's six. A sixth was drafted, covering pages that document a feature without naming its language scope, and cut when its evidence proved wrong: every section I had flagged does carry a scope hint, in a phrasing variant my original search missed. Nothing was added back to restore the count.

Both example rows cite docs findings raised on merged, genuinely documentation-only PRs, so neither row illustrates its change type with a PR of a different type.

Implementation Description

Behavioral contracts

The guide is prose, so its contracts are the claims it makes about this repository. Each is separately checkable.

  1. No CI job reads any file under docs/content.
  2. The site build is triggered on a schedule and by manual dispatch, not on pull requests.
  3. Ruff runs with python/ as its root, so it does not reach docs/.
  4. No link checker runs anywhere in the repository.
  5. Nothing compiles, imports, or runs the code inside a documentation page.
  6. weight orders a page among the pages it sits beside.
  7. Each of the two cited review threads raised what the guide's table says it raised.

Failure behavior

There is no runtime failure path. The failure mode is a wrong claim, and nothing in CI would catch one: RAT excludes docs and review-guides, no markdown linter exists, and the relative link in code_review.md is checked by nothing.

That makes a false sentence here worse than a false sentence in a comment, because it is guidance a reviewer acts on. One such claim did occur and is worth stating. An earlier draft said no CI job reads anything under docs/. That is false: SchemaParityTest and test_specs.py both read docs/yaml-schema.json, so a PR touching only that file is documentation-only by path and is validated byte for byte. Contract 1 is now scoped to docs/content, verified separately, with the only references to that path outside the subtree being Hugo's own settings in docs/config.toml.

One consequence I did not try to solve here: a docs/yaml-schema.json PR is docs-only by path, so the table routes it to this guide, which is then the wrong guide for it. That seems better settled once the remaining rows are filled than by carving an exception into this one.

Tests

Not applicable in the executable sense, documentation only, no logic. ./tools/check-license.sh passes. No license header is needed: tools/.rat-excludes covers review-guides/, and .licenserc.yaml ignores **/*.md.

Each contract above was verified against source rather than written from memory.

Contract How it was checked
1. No CI job reads docs/content Grepped all workflows, tools/, and every .java, .py, .sh for docs/content and content.zh. Only hits are docs/config.toml, consumed by tools/docs.sh from docs.yml alone
2. Site build trigger docs.yml's on: block is schedule plus workflow_dispatch, no pull_request
3. Ruff root tools/lint.sh, on both its uv and pip paths
4. No link checker Searched all workflows and config, zero hits
5. Page code never run tools/lint.sh, tools/ut.sh, ruff config, pytest config for addopts and doctest_glob
6. weight ordering Documented at docs/README.md
7. Both review threads Both comment bodies re-fetched through the API, both PRs confirmed merged and confirmed docs-only

API

No. No code or public API change, and no compatibility impact for any caller.

For a reader, one row of the code_review.md table changes from planned to a link. The two already-linked rows and the two still planned are untouched, and the Focus cell wording is unchanged from #911.

Documentation

  • doc-included

Was this patch authored or co-authored using generative AI tooling?

  • Yes

Generated-by: Claude Code 2.1.223

Fills the docs-only row of the Change-Type Review Guides table in
code_review.md, following the shape of the existing runtime/ and
Python-Java bridge guides: a focused checklist, a validation section,
and examples drawn from past reviews on merged PRs.

Generated-by: Claude Code 2.1.223
@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Aug 6, 2026
@wenjin272

Copy link
Copy Markdown
Contributor

Thanks for taking this on @weiqingy. The PR itself looks good to me. Regarding our AI-assisted review experiment in #894, however, I found that the detailed PR description added more overhead than it removed for this particular change.

Much of the description records the investigation process, such as discarded drafts and how individual repository facts were verified, rather than the final decisions that need human review. The Behavioral contracts also largely restate the content of docs-only.md; for a document this short, reading the document directly was easier than reviewing a second description and checking that the two remained consistent.

Overall, for a PR of this scope and size, I do not think an additional detailed implementation description is necessary. A brief summary of the purpose, key decisions or limitations, and validation performed would be sufficient.

@wenjin272
wenjin272 merged commit d5e120d into apache:main Aug 10, 2026
28 checks passed
@weiqingy

Copy link
Copy Markdown
Collaborator Author

Thanks for the feedback, it makes sense. Agreed that a detailed description was not worth it for a change this size, and that it should not be restating the document the PR adds.

#965 and #981 are still open and both use the same format, so there are two more samples if you want them. Once those have been through review I will put together the guide PR with all of your feedback folded in.

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

Labels

doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants