Conversation
Growl1234
force-pushed
the
docs-current
branch
9 times, most recently
from
September 16, 2026 14:52
5e5221f to
44d1b10
Compare
Growl1234
marked this pull request as ready for review
September 16, 2026 15:21
There was a problem hiding this comment.
🟡 Changes recommended
The RTD install path can break documentation generation, and contributor guidance remains inconsistent.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Moves generated INPUT reference files out of version control, generates them during Read the Docs builds, and adds documentation guidance.
Changes:
- Adds local documentation build and Markdown guidance.
- Ignores generated reference files.
- Updates RTD and workflow configuration.
File summaries
| File | Summary | Findings |
|---|---|---|
docs/README.md |
Adds documentation guidance. | Moderate (1 vote): local build instructions do not generate the required reference file or build the binary. Moderate (2 votes): use “an ABACUS.” Nit (1 vote): contribution guidance remains inconsistent with the generated-file policy. |
docs/parameters.yaml |
Removes generated parameter metadata. | None. |
docs/conf.py |
Updates Sphinx exclusions. | None. |
docs/.gitignore |
Ignores generated documentation files. | Moderate (2 votes): guidance in docs/CONTRIBUTING.md still describes the files as tracked and required. |
.readthedocs.yaml |
Builds the binary and generates reference documentation. | Critical (1 vote): the relative install prefix can place the binary under build/install while the next command uses the repository-level install path. |
.github/workflows/test.yml |
Removes obsolete consistency validation. | None. |
Review details
Suppressed comments (7)
docs/.gitignore:1
- Ignoring and removing
input-main.mdalso breaks the repository's existing relative Markdown links to that page (for exampledocs/quick_start/input.md:47anddocs/advanced/opt.md:5resolve to a missing file on GitHub). RTD creates the file only during its build, so these links need stable RTD targets or a tracked landing page instead.
advanced/input_files/input-main.md
docs/README.md:26
- Rule: Local documentation build completeness
Severity: error
Location: docs/README.md:22-26
Reason: Sinceinput-main.mdis now ignored and absent from the checkout, it is a requireddocs/advanced/input_files/index.rst:8toctree entry rather than an optional enhancement. These instructions also point to../bin/abacuswithout building or installing that binary, so following the documented steps cannot produce a complete local build.
Suggested action: Make generation mandatory for a complete build and document the build/install command, or accept an explicit path to the installed binary (the RTD recipe usesinstall/bin/abacus).
Exception: not allowed
1. (optional but recommended) Build a ABACUS binary and use it to generate the `parameters.yaml`
file:
```bash
../bin/abacus --generate-parameters-yaml > ./parameters.yaml
docs/README.md:182
- The
alignexample is missing a\\row separator after the A equation. Without it, LaTeX/MathJax does not parse the sample as two aligned equations, so this syntax example renders incorrectly.
$$ \begin{align}
A_{ia,jb} &= (\varepsilon_a^{GW}-\varepsilon_i^{GW})\delta_{ij}\delta_{ab}
B_{ia,jb} &= 2 v_{ia,bj} - W_{ib,aj} \quad .
\end{align} $$
docs/README.md:134
- The repository's root
.pre-commit-config.yaml:1-28has nomdformathook (and the test workflow's pre-commit steps are disabled), so this note incorrectly tells contributors that a pre-commit formatter will enforce the1.style. Either add and wire the hook or describemdformatas an optional tool instead.
> Every item uses `1.` intentionally in the markdown source file, and the formatting tool in the
> precommit check will apply this style if detected. The list will still be rendered with the
> intended numbers as indices on github and the final HTML documentation page, but there is no
> longer the need to track and edit the numbers manually. For more information, see `mdformat` docs
> on [ordered lists](https://mdformat.readthedocs.io/en/stable/users/style.html#ordered-lists).
docs/README.md:26
../bin/abacusis not produced by the repository's normal CMake build: the executable is emitted in the build tree with a configuration-dependent name, and thebin/abacussymlink is only created by an install step. This command fails in a normal checkout unless a separate installation has been made; document the required build/install path or use a path supplied by the contributor.
../bin/abacus --generate-parameters-yaml > ./parameters.yaml
docs/README.md:22
- These generation steps cannot be optional after the generated files are removed from the repository:
docs/advanced/input_files/index.rstalways toctreesinput-main, so a clean checkout that skips them produces an incomplete documentation tree. Make generation required for a full local build, or have the build invoke it automatically.
1. (optional but recommended) Build a ABACUS binary and use it to generate the `parameters.yaml`
docs/README.md:26
- Rule: Documentation sync
Severity: error
Location: docs/README.md:22-26
Reason: This PR changes the policy soparameters.yamlandinput-main.mdare generated and ignored, butdocs/CONTRIBUTING.md:177-210still says these files are checked in, must be included in every INPUT-parameter PR, and are compared by CI. Contributors will receive contradictory instructions and may try to commit files that Git now ignores.
Suggested action: Update the contribution guide to describe the generated-only/CI workflow and remove the checked-in-file and mismatch-rejection instructions.
Exception: not allowed
1. (optional but recommended) Build a ABACUS binary and use it to generate the `parameters.yaml`
file:
```bash
../bin/abacus --generate-parameters-yaml > ./parameters.yaml
- Files reviewed: 5/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Growl1234
force-pushed
the
docs-current
branch
from
September 16, 2026 15:45
44d1b10 to
fdc1999
Compare
Growl1234
force-pushed
the
docs-current
branch
from
September 16, 2026 16:08
fdc1999 to
8c439aa
Compare
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.
Generate the input reference documentation during CI workflows instead of tracking the generated
parameters.yamlandinput-main.mdfiles in the repository. This avoid the maintenance of two generated files; contributors no longer need to re-generate the yaml and markdown when changing input reference.Add a
docs/README.mdto describe the guidance of building documentation and some Markdown syntax to help contribution to the documentation.Wrt. the CI workflow design: I tried to let Test workflow generate markdown file from built binary and pass it to RTD workflow, but it adds a few complexity and several attempts failed. Therefore, currently RTD workflow is building a minimal binary itself, which makes RTD workflow around 6 min longer.