-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add pre-commit hook #40
Conversation
Check the migrations files in the current repository and ensure that they all have a `safe` property defined. While we can think of easy ways to break this implementation, for all the use-cases I've had this covers it quite nicely so I think it's worth merging even if its rough. Tim Schilling put an initial sketch of this in #39 and I was able to modify it for use in a precommit hook without difficulty. The modifications were made to tailor it to the environment of pre-commit, where file names are passed to the command as arguments. This allows pre-commit to only need to run the checks incrementally for the affected files for new commits. Co-authored-by: Tim Schilling <schillingt@better-simple.com>
3dc85d7
to
4e76e8c
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 4 +1
Lines 71 90 +19
Branches 24 29 +5
=========================================
+ Hits 71 90 +19
☔ View full report in Codecov by Sentry. |
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.
Could we add how to install this to the README? It will help adoption from other folks.
Yes, although because the configuration has a release version in it, we'll have to update that version when we do releases. Do you think that's acceptable? |
Yeah that seems reasonable. I should probably document that whole process anyway. |
OK, I updated it, but I put a commit hash. Would you like me to go ahead and prepare a release? |
Ug, I got my wires crossed. I have it as described locally, but the rst linter isn't happy and I need to fix it. I'll plan to prepare a release unless you say you prefer I not. |
That should be fine. |
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.
Looks good! Thank you for pushing this forward
Alright, I've tagged the release. It actually probably doesn't matter if it gets to PyPI since it's for the pre-commit hook, which is directly on the repository. It's already working well for me. But if you do want to cut the release to match the tag, you'll need to do it. I no longer have maintainer access to the package on PyPI. |
Done! |
thank you! |
Check the migrations files in the current repository and ensure that they all have a
safe
property defined. While we can think of easy ways to break this implementation, for all the use-cases I've had this covers it quite nicely so I think it's worth merging even if its rough.Tim Schilling put an initial sketch of this in #39 and I was able to modify it for use in a precommit hook without difficulty. The modifications were made to tailor it to the environment of pre-commit, where file names are passed to the command as arguments. This allows pre-commit to only need to run the checks incrementally for the affected files for new commits.
Fix #39