-
Notifications
You must be signed in to change notification settings - Fork 3
[WIP] Fix quick setup issues in code #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
Copilot
AI
Dec 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Dec 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.