Skip to content

Conversation

@mulhod
Copy link
Contributor

@mulhod mulhod commented Dec 21, 2020

Hooks are installed as a development dependency, so this shouldn't affect anything in terms of packaging.

I have left out black for 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.

@codecov
Copy link

codecov bot commented Dec 21, 2020

Codecov Report

Merging #650 (bcedc8e) into main (29614f3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #650   +/-   ##
=======================================
  Coverage   95.06%   95.06%           
=======================================
  Files          27       27           
  Lines        3098     3098           
=======================================
  Hits         2945     2945           
  Misses        153      153           
Impacted Files Coverage Δ
...utils/commandline/compute_eval_from_predictions.py 97.18% <ø> (ø)
skll/utils/commandline/plot_learning_curves.py 96.15% <ø> (ø)
skll/__init__.py 100.00% <100.00%> (ø)
skll/config/__init__.py 95.09% <100.00%> (ø)
skll/data/__init__.py 100.00% <100.00%> (ø)
skll/data/readers.py 90.13% <100.00%> (ø)
skll/data/writers.py 94.11% <100.00%> (ø)
skll/experiments/__init__.py 95.19% <100.00%> (ø)
skll/experiments/output.py 97.36% <100.00%> (ø)
skll/experiments/utils.py 93.45% <100.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29614f3...bcedc8e. Read the comment docs.

@pep8speaks
Copy link

pep8speaks commented Dec 21, 2020

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

@mulhod
Copy link
Contributor Author

mulhod commented Dec 22, 2020

Addresses #646

@mulhod mulhod changed the title [WIP] Introduce pre-commit hooks Introduce pre-commit hooks Dec 22, 2020
@mulhod mulhod requested review from a user, aoifecahill, desilinguist and jimmybru and removed request for jimmybru December 22, 2020 03:26
Copy link
Collaborator

@desilinguist desilinguist left a 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?

Copy link
Collaborator

@desilinguist desilinguist left a 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.

@mulhod
Copy link
Contributor Author

mulhod commented Jan 4, 2021

Should we exclude the CSV/TSV/JSON files from the test data from the auto-formatting fixes?

I have gone ahead and excluded these files from the EOF file hook, which drastically lessens the number of files that have changes.

@mulhod mulhod requested a review from desilinguist January 4, 2021 21:53
Copy link
Collaborator

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link

@ghost ghost left a 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,
Copy link

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?

@desilinguist desilinguist merged commit 32823c0 into main Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants