-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix flaky pre-commit install, via nuclear option, dont try to install… #24
Conversation
Reviewer's Guide by SourceryThis PR modifies the pre-commit hook installation process to make it more robust and configurable. The main changes include making package installation optional, improving symlink handling, and adding better error handling and user feedback. Updated class diagram for install_ipd_pre_commit_hook functionclassDiagram
class ProjectConfig {
+install_ipd_pre_commit_hook(projdir, path=None, install_packages=False)
+ensure_pre_commit_packages()
}
note for install_ipd_pre_commit_hook "Improved symlink handling, optional package installation, and error handling."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Summary
|
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.
Hey @willsheffler - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider enhancing the error handling to include the actual exception message rather than just printing 'error installing pre-commit hook' to help with debugging
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
print('pre-commit hook installed') | ||
else: | ||
print('pre-commit hook installed, but packages not installed') | ||
except RuntimeError: |
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.
suggestion (bug_risk): Consider catching more specific exceptions and including error details in the message
Catching RuntimeError is too broad and could mask specific issues. Consider catching specific exceptions (like PermissionError, FileNotFoundError) and including the error details in the message.
except (PermissionError, FileNotFoundError) as e:
print(f'error installing pre-commit hook: {str(e)}')
@@ -11,6 +11,8 @@ def test_install_ipd_pre_commit_hook(tmpdir): | |||
os.system('git init') | |||
ipd.dev.install_ipd_pre_commit_hook('.') |
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.
issue (testing): Test needs to verify new install_packages parameter behavior
The function has been updated to include an install_packages parameter, but the test doesn't verify both True and False scenarios. Consider adding assertions to check that packages are installed or not installed based on this parameter.
… deps
Summary by Sourcery
Improve the reliability of the pre-commit hook installation by handling broken symlinks and adding error handling. Introduce an option to skip package installation during the hook setup. Enhance tests to ensure the idempotency of the installation process.
Bug Fixes:
Enhancements:
Tests: