Skip to content

Introduce pre-commit hooks - #650

Merged
desilinguist merged 8 commits into
mainfrom
pre-commit
Jan 5, 2021
Merged

Introduce pre-commit hooks#650
desilinguist merged 8 commits into
mainfrom
pre-commit

Conversation

@mulhod

@mulhod mulhod commented Dec 21, 2020

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Dec 21, 2020

Copy link
Copy Markdown

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

pep8speaks commented Dec 21, 2020

Copy link
Copy Markdown

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

mulhod commented Dec 22, 2020

Copy link
Copy Markdown
Contributor Author

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

@desilinguist desilinguist left a comment

Copy link
Copy Markdown
Collaborator

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 desilinguist left a comment

Copy link
Copy Markdown
Collaborator

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

mulhod commented Jan 4, 2021

Copy link
Copy Markdown
Contributor Author

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

@desilinguist desilinguist left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@ghost ghost left a comment

Copy link
Copy Markdown

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.

Comment thread skll/learner/__init__.py
GradientBoostingClassifier,
GradientBoostingRegressor,
RandomForestClassifier,
RandomForestRegressor,

Copy link
Copy Markdown

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?

Comment thread skll/learner/__init__.py
Comment thread skll/learner/__init__.py
Comment thread skll/learner/utils.py
@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.

4 participants