-
Notifications
You must be signed in to change notification settings - Fork 68
Introduce pre-commit hooks #650
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
Codecov Report
@@ Coverage Diff @@
## main #650 +/- ##
=======================================
Coverage 95.06% 95.06%
=======================================
Files 27 27
Lines 3098 3098
=======================================
Hits 2945 2945
Misses 153 153
Continue to review full report at Codecov.
|
|
Hello @mulhod! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🎉 Comment last updated at 2020-12-22 03:23:30 UTC |
|
Addresses #646 |
desilinguist
left a comment
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.
Should we exclude the CSV/TSV/JSON files from the test data from the auto-formatting fixes?
desilinguist
left a comment
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.
Everything else looks good to me! This will be quite useful! Just that one minor suggestion.
…n files for that hook
I have gone ahead and excluded these files from the EOF file hook, which drastically lessens the number of files that have changes. |
desilinguist
left a comment
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.
LGTM!
ghost
left a comment
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 have a comment, but seems like that is how black does.
Else everything looks good to me.
| GradientBoostingClassifier, | ||
| GradientBoostingRegressor, | ||
| RandomForestClassifier, | ||
| RandomForestRegressor, |
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.
do we need a comma at the end?
Hooks are installed as a development dependency, so this shouldn't affect anything in terms of packaging.
I have left out
blackfor now because I don't want this getting too big and controversial. I will do that next separately so that we can all see what exactly will happen.