CI build documentation automatically#120
Conversation
Also fixes other errors for the default config
There was a problem hiding this comment.
Code Review
This pull request replaces the external rpl dependency in remove_keys.py with a native Python implementation for sanitizing documentation files, updates version comparison logic to use looseversion, and adds several documentation dependencies to pyproject.toml. Review feedback suggests improving the sanitization script by handling missing configuration keys with fallbacks, restoring case-insensitive replacement to prevent sensitive data exposure, simplifying the should_replace logic, and using safer error handling for file encoding to prevent silent data loss.
| def sanitize_file(file_path, replacements): | ||
| global files_sanitized, files_changed | ||
| try: | ||
| with open(file_path, "r", encoding="utf-8", errors="ignore") as handle: |
There was a problem hiding this comment.
Using errors="ignore" when opening files for reading and writing (see also line 51) can lead to silent data loss if the files contain characters that are not valid UTF-8. For a tool that modifies files in-place, it is safer to use errors="replace" to preserve the file structure or to omit the parameter to allow the script to fail loudly if an encoding issue occurs, preventing silent corruption of documentation files.
References
- The 'errors' parameter in the open() function is a Python 3 feature that allows for explicit encoding error handling.
There was a problem hiding this comment.
This doesn't seem very important, but I did change it anyway. I don't think it has any implications for leaking keys.
There was a problem hiding this comment.
Pull request overview
This PR adds a new GitHub Actions workflow that builds Sphinx documentation on every push/PR to main and publishes the built site to the gh-pages branch on tag pushes (addressing issue #104). It also rewrites the doc-key scrubber in pure Python so secrets are no longer echoed into Action logs, and aligns LooseVersion/pip caching with modern packaging.
Changes:
- Add
.github/workflows/build_docs.ymlto build docs on push/PR and deploy togh-pageson tags. - Replace the
rpl-shellinggendoc/remove_keys.pywith a pure-Python walker that does not print secrets, plus an empty-args guard. - Switch
distutils.version.LooseVersionto thelooseversionpackage, add correspondingdocsdependencies, and update the test workflow's pip cache key topyproject.toml.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/build_docs.yml |
New workflow building Sphinx docs and conditionally publishing to gh-pages. |
.github/workflows/run_tests.yml |
Update pip cache key from setup.py to pyproject.toml. |
pyproject.toml |
Add looseversion, numpydoc, and sphinx_bootstrap_theme to the docs dependency group. |
gendoc/tools/build_modref_templates.py |
Switch LooseVersion import to the maintained looseversion package. |
gendoc/remove_keys.py |
Replace rpl-based shell calls with a Python file walker; add empty-args guard and error reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gallantlab/cottoncandy/sessions/9682fb4b-670f-4c5c-ad82-3ec96d1cdead Co-authored-by: kroq-gar78 <648148+kroq-gar78@users.noreply.github.com>
Added manual trigger for publishing documentation.
|
LGTM. I added an option to build and publish docs manually (it can be helpful). Will merge after CI passes. |
This PR adds a new workflow to build documentation and push to GitHub Pages on tagged commits. (Fixes #104.)
It also replaces the previous key scrubbing script to avoid leaking keys in the Actions logs (
gendoc/remove_keys.py). I've confirmed locally that it doesn't output any keys.