make 'vale' check optional locally - #1708
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a strict Vale wrapper, updates the local pre-commit hook to use it, and excludes the wrapper from four CI changed-file groups. ChangesVale integration
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change makes Vale optional for local contributors while keeping the check required in CI; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 140-143: Update the Vale hook configuration around entry
ci/run-vale.sh so it receives and lints the matched documentation file paths:
remove pass_filenames: false, or explicitly configure args to target
docs/cuopt/source. Preserve the existing verbose and system-language settings.
In `@ci/run-vale.sh`:
- Around line 8-17: Add unit tests covering the three branches in the Vale
wrapper: missing vale with CI unset exits successfully, missing vale with
CI=true exits unsuccessfully, and an available fake vale receives all arguments
while its exit status is preserved. Ensure tests isolate PATH and CI environment
state and do not depend on a real Vale installation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c827b05c-af5f-4ec3-b572-41628b8227b0
📒 Files selected for processing (3)
.github/workflows/pr.yaml.pre-commit-config.yamlci/run-vale.sh
CI Test Summary2 failed · 29 passed · 0 skipped
|
This comment was marked as resolved.
This comment was marked as resolved.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Description
#1668 added a new
pre-commithook that expects to be able to invokevale(https://github.com/vale-cli/vale).In CI, that tool's provided via the conda environment
pre-commitruns in. Locally, I think it's unlikely that all contributors would have it installed (I don't).This proposes making the
valecheck optional locally but requiring it in CI.Issue
N/A
Checklist
Notes for Reviewers
Options considered
There are other ways to work around this:
goinstalled locallylanguage: condahook (these are heavyweight and slow to initialize, I don't think it's worth it for a docs linter)I think the little shell script is a good compromise.
How I tested this
Made changes I knew would trigger the linter.
In an environment without
vale.And again with it