Repository navigation
PDFCLOUD-6418: Upgrade repository quality scaffolding - #48
datalogics-kam wants to merge 6 commits into
Conversation
✅ Deploy Preview for pdfrest-python ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
867a795 to
9db1576
Compare
|
From Codex
|
datalogics-tsmith
left a comment
datalogics-tsmith
left a comment
There was a problem hiding this comment.
Requesting changes for the two inline findings below: add durable regression coverage for the new filename quality gate, and keep the required contributor hook aligned with the repository's Python 3.10 baseline.
| from typing import Any | ||
|
|
||
|
|
||
| def tracked_filename_report(root: Path) -> dict[str, Any]: |
There was a problem hiding this comment.
[P2] Add automated coverage for the checker’s failure paths
This function is now part of the required quality gate, but the PR does not add regression tests for non-NFC paths, normalization collisions, invalid UTF-8, missing Git, or a failed git ls-files invocation. The manual validation described in the PR will not prevent these guarantees from regressing later. Please add focused pytest coverage for the clean and failure cases.
There was a problem hiding this comment.
Agreed on the coverage gap: there are no automated tests for this checker in the current PR, and the manual validation does not provide lasting regression protection. Focused pytest cases for clean paths, non-NFC names, normalization collisions, invalid UTF-8, missing Git, and a failed git ls-files invocation are appropriate for a gate that can block contributor commits. The CLI exit codes and diagnostics should also be asserted so the tests cover the observable hook behavior. I consider this a valid finding to address before merging.
There was a problem hiding this comment.
The NFC checking script is now injected in a location that is marked for not needing review. It's tested in the skill repository.
| @@ -0,0 +1,97 @@ | |||
| # /// script | |||
| # requires-python = ">=3.11" | |||
There was a problem hiding this comment.
[P3] Keep the required hook compatible with Python 3.10
The repository targets Python 3.10–3.14, and this script does not use any Python 3.11-only syntax or APIs. Declaring >=3.11 can require an extra uv-managed interpreter—or warn/fail in restricted 3.10-only contributor environments. Please change this to >=3.10 and update the corresponding README statement.
There was a problem hiding this comment.
The SDK compatibility baseline and the contributor tooling baseline are different here. At this PR's head, .python-version selects Python 3.11, and pre-commit CI explicitly uses 3.11. The SDK remains compatible with 3.10, with the test matrix covering 3.10–3.14. This script is contributor tooling, so its declared 3.11 minimum matches the established development environment.
I also verified that direct execution under Python 3.10 succeeds; explicitly selecting 3.10 through uv succeeds with the metadata warning you described. Lowering the requirement would be a reasonable portability improvement, but a 3.10-only contributor environment is outside the currently configured development baseline. I would treat this as a non-blocking suggestion rather than an SDK compatibility defect, and retain the current declaration and README wording.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Keep service inputs out of mutating formatters and preserve their Git checkout bytes. Add explicit LF and whitespace defaults while retaining Markdown hard breaks and the existing Python attributes. Exclude the uv lockfile from TOML sorting. Assisted-by: Codex
Check JSON syntax, case conflicts, merge markers, private keys, and GitHub Actions workflows. Exempt only the public signing-key fixture from private-key detection. Add a standalone NFC filename checker with an always-run hook and README usage. Resolve Git explicitly and report unavailable Git, invalid UTF-8 paths, and normalization collisions. Assisted-by: Codex
Run basedpyright through the frozen project environment and retire its separate workflow. Run all quality hooks in the shared job with read-only repository permissions, a timeout, pinned actions, and a hook cache keyed by platform, tool versions, and configuration. Assisted-by: Codex
- Vendor the upstream NFC checker with provenance and byte exclusions. - Refresh complete ignore modules while preserving local amendments. - Pin compatible quality tools and CI actions. - Retain the established Ruff rule selection across the tool upgrade. Assisted-by: Codex
- Include Markdown in Ruff formatting while preserving fixture bytes. - Format Python examples in the file-usage guide. Assisted-by: Codex
- Remove the Docs workflow and its GitHub Pages deployment. - Update CI guidance for Docs Check and Netlify hosting. Assisted-by: Codex
aec29c4 to
f29fe21
Compare
This branch was successfully deployed
2 participants
PDFCLOUD-6418
Why this change
The existing quality setup lacked several common gates, ran basedpyright in a separate workflow, and allowed formatters to rewrite uploaded test resources. Apply the targeted common, Python, and shell profiles from cit-repository-quality-scaffolding while preserving the established tools.
What changed (high level)
Keep one pre-commit quality gate, including the project-managed basedpyright environment. Add structural file checks, actionlint, and a read-only tracked-filename UTF-8 NFC checker. Protect service fixtures in hook scopes and Git attributes, retain the Python attributes and complete sourced common module, and document standalone checker usage. Keep the generated uv.lock out of TOML sorting.
Behavior changes
Quality CI runs on Ubuntu/Python 3.11 for pull requests and pushes to main, develop, and feature-*, with contents: read, frozen installs, verified action SHAs, a runtime-aware hook cache, and a 20-minute timeout. Filename problems and normalization collisions now fail the gate without renaming files. The signing-key exception is limited to the public test fixture; fixture JSON still receives syntax checks and maintained Python under resources remains checked.
The existing Python 3.10-3.14 test and example matrices, Python 3.11 PR/scheduled live tests, PR-only diff coverage and docs previews, and publishing are preserved. There are no SDK API or package-version changes.
Reconcile CI with GitHub Pages already being turned off: remove the obsolete Docs workflow, whose Pages setup fails before compilation. Netlify currently hosts python.pdfrest.com. Documentation validation remains in Test and Publish through Docs Check (
uv run mkdocs build --stricton Python 3.11), including PR preview artifacts and the documentation gate before package publishing. Update the contributor CI guidance to reflect the hosting setup.Validation
The independent branch snapshot based on upstream/main passed the full pre-commit suite, Ruff lint and format checks on all 123 Python files, and basedpyright with zero errors or warnings. All hook revisions/IDs and quality-workflow action pins were verified through gh. Six NFC scenarios and disposable-repository tests for formatter exclusions, maintained Python coverage, fixture JSON validation, and LF-only CSV checkout preservation passed. The original checkout's 44 resource hashes and unrelated local edits were preserved; repeat inspection/rendering showed no source drift. The GitHub-Markdown formatting hook had no matching files.
For the Pages workflow removal, the strict MkDocs build, remaining GitHub workflow validation, Markdown formatting, commit hooks, and diff checks passed. SDK pytest/Nox, live tests, examples, and package builds were not rerun because the change affects CI configuration and guidance. Repository-wide Ruff in the original checkout still reports 39 existing undefined names in its untracked demo notebook; that file is absent from this PR and remains untouched. Hosted CI results are pending.
Risks and follow-ups
Basedpyright is now included in the pre-commit status instead of a separate workflow. Inspected branch rules did not require the retired status, and no GitHub settings were changed. Broader gitignore refreshes and unrelated tool-version upgrades are deferred.