-
Notifications
You must be signed in to change notification settings - Fork 47
Intro git hooks for typechecking #689
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
| Returns | ||
| ------- | ||
| None | ||
| JSONType |
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.
This and the other file change are just two minor changes to test that it was grabbing the right files
| @@ -0,0 +1,111 @@ | |||
| #!/bin/bash | |||
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.
| #!/bin/bash | |
| #!/bin/env bash |
☝🏻 better x-plat support on different machines nowadays. More of a good practice thing than anything because who doesn't have bash?
| # Make sure these tools are installed and available in your system's PATH before running the script. | ||
|
|
||
| # Define the hook script | ||
| HOOK_SCRIPT="#!/bin/bash |
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.
| HOOK_SCRIPT="#!/bin/bash | |
| HOOK_SCRIPT="#!/bin/env bash |
Same again, sorry, but I put a suggestion in so that you can accept or reject it easily!
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.
I'd also probably EOF this, but if it ain't broke.
| echo -e '\nRunning Black' | ||
| echo ---------------------------------------- | ||
| BLACK_FAILED=0 | ||
| black --check \$FILES || BLACK_FAILED=1 |
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.
These could each be like
BLACK_FAILED=`black --check \$FILES`and so on, with the same function, but again, if it ain't broke.
| # Check if any linter failed | ||
| echo Summary | ||
| echo ---------------------------------------- | ||
| if [ \$BLACK_FAILED -eq 1 ]; then |
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.
Consider escaped quotes around comparison variables. Prevents them going wrong in certain scenarios. Again, it's a good practice > required.
| echo "Unknown option: $key" | ||
| exit 1 | ||
| ;; | ||
| esac |
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.
NGL, that's pretty nice case statement
Problem
We now have linting and typechecking on the darwin-py future code but no non-manual way to ensure this code is up to scratch before creating a PR
Solution
Add in a typechecking optional pre/post commit hook script
install via
./.hooks/typechecking_installer.sh -o/-p-o default for post-commit (non-blocking warning) vs -p pre-commit (strictly enforced),Changelog
Optional developer installable git hooks