[WIP] Fix quick setup issues in code#57
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
The grep pattern now includes both sphinx and pre-commit packages. Also added error handling if pre-commit is not found. Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
…ndencies Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes the setup_dev.sh script to properly handle pre-commit installation and improves error handling when pre-commit is unavailable after installation attempts.
Key Changes:
- Updated the grep pattern to extract both sphinx and pre-commit dependencies from requirements-dev.txt
- Added conditional check with error handling when pre-commit command is not available
- Cleaned up trailing whitespace in CONTRIBUTING.md and setup_dev.bat
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| setup_dev.sh | Updated dependency extraction logic to include pre-commit, renamed variables for clarity, and added error handling for missing pre-commit command |
| setup_dev.bat | Removed trailing newline |
| CONTRIBUTING.md | Removed trailing whitespace on line 193 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "This should have been installed from requirements-dev.txt." | ||
| echo "Please ensure pre-commit is listed in requirements-dev.txt or install it manually:" | ||
| echo " pip install pre-commit" |
There was a problem hiding this comment.
The error handling assumes that pre-commit should have been installed from requirements-dev.txt, but the script only attempts to install it if requirements-dev.txt exists AND contains a line matching the grep pattern. If requirements-dev.txt doesn't exist or doesn't contain pre-commit, the error message will be misleading. Consider checking if requirements-dev.txt exists and contains pre-commit before assuming it should have been installed from there.
| echo "This should have been installed from requirements-dev.txt." | |
| echo "Please ensure pre-commit is listed in requirements-dev.txt or install it manually:" | |
| echo " pip install pre-commit" | |
| if [ -f "requirements-dev.txt" ] && grep -q -E '^[[:space:]]*pre-commit' requirements-dev.txt; then | |
| echo "pre-commit appears to be listed in requirements-dev.txt but was not installed." | |
| echo "You may need to rerun this script or install it manually:" | |
| echo " pip install pre-commit" | |
| else | |
| echo "pre-commit is not installed and does not appear to be listed in requirements-dev.txt." | |
| echo "Please add pre-commit to your development dependencies (e.g., requirements-dev.txt) or install it manually:" | |
| echo " pip install pre-commit" | |
| fi |
| echo "❌ Error: pre-commit is not installed in this environment." | ||
| echo "This should have been installed from requirements-dev.txt." | ||
| echo "Please ensure pre-commit is listed in requirements-dev.txt or install it manually:" | ||
| echo " pip install pre-commit" | ||
| exit 1 |
There was a problem hiding this comment.
The exit 1 here will terminate the script if pre-commit is not available, which may be too strict. If pre-commit is truly optional or can be installed later, consider making this a warning instead of a fatal error. Alternatively, the script could attempt to install pre-commit directly if it's missing.
| echo "❌ Error: pre-commit is not installed in this environment." | |
| echo "This should have been installed from requirements-dev.txt." | |
| echo "Please ensure pre-commit is listed in requirements-dev.txt or install it manually:" | |
| echo " pip install pre-commit" | |
| exit 1 | |
| echo "⚠️ Warning: pre-commit is not installed in this environment." | |
| echo " Pre-commit hooks will not be installed automatically." | |
| echo " To enable pre-commit hooks, ensure 'pre-commit' is listed in requirements-dev.txt" | |
| echo " or install it manually in this environment with:" | |
| echo " pip install pre-commit" |
Summary
Fixed the
setup_dev.shscript to properly handle pre-commit installation from requirements-dev.txt.Changes Made:
CI Failures Note:
The pre-commit CI workflow is failing due to 214 pre-existing mypy type annotation errors in test files, examples, and benchmarks. These errors existed before this PR and are caused by
disallow_untyped_defs = truein pyproject.toml. The setup script changes are correct and address the issues identified in code review comments.Security Summary:
No security vulnerabilities were introduced or discovered during this change.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.