Feat/quick setup #56
Feat/quick setup #56
Conversation
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Pin pre-commit version and align installation logic between bash and Windows setup scripts
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdded pre-commit to dev requirements, removed pip fallback installs from POSIX and Windows dev setup scripts, adjusted POSIX script to extract and install both Sphinx and pre-commit extras and to require the Changes
Sequence Diagram(s)(omitted — changes are script and dependency updates without multi-component control-flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
bug
There was a problem hiding this comment.
Pull request overview
This PR attempts to simplify the pre-commit setup process by adding pre-commit as an explicit dependency in requirements-dev.txt and removing conditional installation logic from the development setup scripts. However, there are critical issues with this implementation that will cause setup failures on Unix/Linux systems.
Key changes:
- Added pre-commit==4.5.1 to requirements-dev.txt
- Removed pre-commit installation logic from setup_dev.sh (conditional checks and pip install)
- Removed pre-commit pip installation from setup_dev.bat
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| setup_dev.sh | Simplified pre-commit hook installation by removing conditional checks for pre-commit availability |
| setup_dev.bat | Removed explicit pip install for pre-commit, assuming it will be installed via requirements-dev.txt |
| requirements-dev.txt | Added pre-commit version 4.5.1 as an explicit development dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
setup_dev.sh
Outdated
| pre-commit install | ||
| echo "✅ Pre-commit hooks installed" | ||
| fi | ||
| pre-commit install |
There was a problem hiding this comment.
The error handling has been removed when calling pre-commit install. If pre-commit is not installed or if the installation fails, the script will exit immediately due to the 'set -e' at line 6, but there won't be a user-friendly error message explaining what went wrong. Consider adding error checking with a clear error message like the original code had.
| pre-commit install | |
| if ! command -v pre-commit &> /dev/null; then | |
| echo "❌ Error: pre-commit is not installed in this environment." | |
| echo "Please install it (e.g., inside the virtual environment) and re-run this script:" | |
| echo " pip install pre-commit" | |
| exit 1 | |
| fi | |
| if ! pre-commit install; then | |
| echo "❌ Error: Failed to install pre-commit hooks." | |
| echo "Please check the pre-commit configuration and try running:" | |
| echo " pre-commit install" | |
| exit 1 | |
| fi |
| @@ -73,15 +73,8 @@ fi | |||
| # Install pre-commit hooks | |||
| echo "" | |||
| echo "🔧 Setting up pre-commit hooks..." | |||
There was a problem hiding this comment.
The pre-commit command will fail on Unix/Linux systems because pre-commit is not being installed. The setup_dev.sh script only installs sphinx-related dependencies from requirements-dev.txt (see lines 64-68 which use grep to filter only sphinx packages), so adding pre-commit to requirements-dev.txt won't actually install it. You need to either:
- Add pre-commit to the [dev] optional dependencies in pyproject.toml, or
- Update the grep filter in setup_dev.sh to also include pre-commit packages, or
- Explicitly install pre-commit before calling pre-commit install
| echo "🔧 Setting up pre-commit hooks..." | |
| echo "🔧 Setting up pre-commit hooks..." | |
| echo "📥 Ensuring pre-commit is installed..." | |
| if ! command -v pre-commit >/dev/null 2>&1; then | |
| pip install pre-commit --quiet | |
| fi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
setup_dev.sh (1)
1-1: Minor: Fix end-of-file formatting.The pre-commit end-of-file-fixer hook detected a missing newline at the end of the file. This has been auto-fixed by the hook, but you may need to commit the change.
Based on pipeline failures.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
requirements-dev.txtsetup_dev.batsetup_dev.sh
💤 Files with no reviewable changes (1)
- setup_dev.bat
🧰 Additional context used
🪛 GitHub Actions: Pre-commit
setup_dev.sh
[error] 1-1: End-of-file-fixer hook modified the file 'setup_dev.sh'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
|
@copilot the ci are failing fix it |
|
@omsherikar I've opened a new pull request, #57, to work on those changes. Once the pull request is ready, I'll request review from you. |
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>
[WIP] Fix quick setup issues in code
|
@copilot still CI failing 7s Successfully installed astroid-3.3.11 click-8.1.8 colorama-0.4.6 libcst-1.8.6 mando-0.7.1 markdown-it-py-3.0.0 mdurl-0.1.2 pygments-2.19.2 pyyaml-6.0.3 radon-6.0.1 refactron-1.0.0 rich-14.2.0 six-1.17.0 typing-extensions-4.15.0 |
|
@omsherikar I've opened a new pull request, #58, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Fix pre-commit version incompatibility with Python 3.8-3.9
|
@copilot still the ci failing |
|
@omsherikar I've opened a new pull request, #59, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Fix pre-commit version constraint for Python 3.8-3.9 compatibility
…another-one Revert "Fix pre-commit version constraint for Python 3.8-3.9 compatibility"
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
requirements-dev.txt
🧰 Additional context used
🪛 GitHub Actions: CI
requirements-dev.txt
[error] 1-1: Command failed: pip install -r requirements-dev.txt. No matching distribution found for pre-commit==4.3.0
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Pull Request
📋 Description
A clear and concise description of what this PR does.
🔗 Related Issue
Closes #(issue number)
🧪 Type of Change
🧪 Testing
📝 Changes Made
🎯 Code Quality
📚 Documentation
🔒 Security
🚀 Performance
📋 Checklist
🎨 Screenshots (if applicable)
Add screenshots to help explain your changes.
📋 Additional Notes
Add any other context about the PR here.
🔍 Reviewers
@omsherikar - Please review this PR
🏷️ Labels
Please add appropriate labels to this PR:
bug- Bug fixenhancement- New featuredocumentation- Documentation updatedependencies- Dependency updatesecurity- Security updateperformance- Performance improvementrefactoring- Code refactoringtesting- Test updatesbreaking-change- Breaking changegood-first-issue- Good for new contributorshelp-wanted- Help neededpriority-high- High prioritypriority-medium- Medium prioritypriority-low- Low prioritySummary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.