quick setup added for Mac/Linux and Windows#53
Conversation
github-actions
bot
added
bug
|
Warning Rate limit exceeded@omsherikar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
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. WalkthroughAdds cross-platform automated development setup scripts ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
1-1: Remove trailing whitespace (flagged by pre-commit).The pre-commit hook detected trailing whitespace that needs to be removed. This should be automatically fixed by running the pre-commit hooks.
Run
pre-commit run --all-filesto fix this issue automatically.
🧹 Nitpick comments (3)
setup_dev.sh (2)
71-80: Usepythoninstead ofpython3inside the virtual environment.Since the virtual environment was activated at line 40, using
pythonis more reliable thanpython3. Thepythoncommand inside an activated venv is guaranteed to point to the correct Python executable, whereaspython3depends on system symlinks that may not exist everywhere.Apply this diff:
echo "🧪 Verifying installation..." -# Use python3 for consistency (venv should have it, but python3 is more reliable) -if python3 -c "import refactron; print('✅ Refactron imported successfully')" 2>/dev/null; then +if python -c "import refactron; print('✅ Refactron imported successfully')" 2>/dev/null; then echo "✅ Installation verified" else echo "❌ Error: Could not import refactron"
93-104: Consider removing the-xflag from pytest to show all test results.The
-xflag stops pytest at the first failure, which might hide other test issues during setup verification. Running all tests would give developers a complete picture of the test suite status.Apply this diff:
echo "🧪 Running tests to verify setup..." if pytest --version &> /dev/null; then - pytest tests/ -v --tb=short -x || { + pytest tests/ -v --tb=short || { echo "" echo "⚠️ Some tests failed, but setup is complete." echo "You can investigate test failures later."setup_dev.bat (1)
96-110: Consider removingpausefor consistency with the Unix script.The
pausecommand at line 110 blocks execution until the user presses a key, which differs from the bash script behavior. This can cause issues if the script is run from CI/CD, another script, or automation tools.Apply this diff to remove the pause:
echo 💡 To activate the environment in the future, run: echo venv\Scripts\activate echo. echo Happy coding! 🚀 -pauseAlternatively, make it conditional:
echo Happy coding! 🚀 + +REM Pause only if run interactively (optional) +if /I "%1"=="--no-pause" goto :eof pause
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CONTRIBUTING.md(1 hunks)README.md(1 hunks)setup_dev.bat(1 hunks)setup_dev.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pre-commit
CONTRIBUTING.md
[error] 1-1: Trailing whitespace detected and fixed by pre-commit (hook: trailing-whitespace).
setup_dev.sh
[error] 1-1: End-of-file newline issue fixed by pre-commit (hook: end-of-file-fixer).
⏰ 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
🔇 Additional comments (8)
README.md (1)
87-88: LGTM!The updated documentation clearly directs contributors to the appropriate setup script for their platform.
setup_dev.sh (4)
1-6: LGTM! Script header is properly configured.The shebang and
set -eare appropriate for a setup script. Note thatset -ewill exit on any error, which works well with the error handling patterns used throughout the script.
18-23: LGTM! Python version validation is correct.The script properly validates that Python 3.8+ is available, which aligns with the project requirements.
58-69: LGTM! Pre-commit setup is robust.The script properly checks for pre-commit availability and installs it if needed, ensuring hooks are set up correctly.
47-50: The[dev]extras are properly defined in pyproject.toml and the installation command is correct.The package metadata includes the
[dev]optional-dependencies with pytest, pytest-cov, black, and mypy, so thepip install -e ".[dev]"command on line 50 will successfully install all development dependencies.setup_dev.bat (2)
75-94: LGTM! Installation verification is thorough.The script properly verifies both the Python import and CLI functionality, with good error handling and user feedback.
60-73: LGTM! Pre-commit setup works correctly.The script installs pre-commit and sets up hooks with appropriate error handling. While the bash script checks if pre-commit is already available before installing, always installing is a safe approach that ensures consistency.
CONTRIBUTING.md (1)
50-67: LGTM! Manual setup instructions are clear and accurate.The manual setup steps properly document the alternative approach for developers who prefer not to use the automated scripts.
| echo " source venv/bin/activate" | ||
| echo "" | ||
| echo "Happy coding! 🚀" | ||
|
|
There was a problem hiding this comment.
Add newline at end of file (flagged by pre-commit).
The pre-commit hook detected that the file is missing a trailing newline, which is required by POSIX standards for text files.
Add a blank line after line 120 to fix this issue.
🤖 Prompt for AI Agents
In setup_dev.sh around line 120, the file is missing a trailing newline required
by POSIX; open the file and add a single blank line (press Enter) after line 120
so the file ends with a newline character, then save and re-run pre-commit.
There was a problem hiding this comment.
Pull request overview
This PR adds automated development environment setup scripts to simplify the onboarding process for contributors. Instead of manually running multiple commands to set up a virtual environment, install dependencies, and configure pre-commit hooks, developers can now run a single script that automates the entire setup process.
Key Changes:
- Added
setup_dev.shfor macOS/Linux that automates virtual environment creation, dependency installation, pre-commit hook setup, and verification tests - Added
setup_dev.batfor Windows with similar automation but without test execution - Updated README.md and CONTRIBUTING.md to promote the automated scripts as the recommended setup method while keeping manual instructions available
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| setup_dev.sh | New bash script that automates development environment setup for macOS/Linux, including Python version validation, virtual environment creation, dependency installation, pre-commit hooks, and test execution |
| setup_dev.bat | New batch script that automates development environment setup for Windows, including Python detection, virtual environment creation, dependency installation, and pre-commit hooks (missing Python version validation and test execution) |
| README.md | Updated Quick Start section to reference the new setup scripts as the primary installation method |
| CONTRIBUTING.md | Updated Development Setup section to recommend automated scripts while preserving manual setup instructions as an alternative |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if command -v pre-commit &> /dev/null || pip show pre-commit &> /dev/null; then | ||
| pre-commit install | ||
| echo "✅ Pre-commit hooks installed" | ||
| else | ||
| echo "⚠️ pre-commit not found. Installing..." | ||
| pip install pre-commit --quiet | ||
| pre-commit install | ||
| echo "✅ Pre-commit hooks installed" | ||
| fi |
There was a problem hiding this comment.
The pre-commit installation logic differs between the bash and Windows scripts. The bash script checks if pre-commit exists before installing (lines 61-69), while the Windows script always installs it (setup_dev.bat line 63). For consistency, consider either always installing pre-commit (since pip install -e ".[dev]" doesn't include it in pyproject.toml) or checking for it in both scripts. The current approach in the bash script is correct since pre-commit is not in the dev extras.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
87-89: Consider clarifying the Windows invocation syntax.Line 88 shows the Windows batch file only as a comment (
# OR setup_dev.bat). While the intent is clear to experienced developers, Windows users might benefit from slightly more explicit guidance—either by showing it as a proper code example or by clarifying that they should runsetup_dev.bat(withoutbash).Current example works but could be marginally clearer:
bash setup_dev.sh # macOS/Linux setup_dev.bat # Windows (remove 'bash', run batch directly)This is a minor UX improvement for the documentation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🔇 Additional comments (1)
README.md (1)
87-88: Execute permission issue has been addressed.Good catch incorporating the earlier feedback—using
bash setup_dev.shworks correctly regardless of file execute permissions. This is the right approach.
|
@omsherikar I've opened a new pull request, #54, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@omsherikar I've opened a new pull request, #55, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@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
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.