Skip to content
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

Make Travis check formatting with Black #215

Merged
merged 2 commits into from
Jun 23, 2019

Conversation

tomwhite
Copy link
Collaborator

@tomwhite tomwhite commented Apr 3, 2019

Black has been used to format the source previously. This adds a check and fails the build if the source is not correctly formatted. PR authors can fix the formatting by running black umap.

@pep8speaks
Copy link

pep8speaks commented Apr 3, 2019

Hello @tomwhite! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 269:80: E501 line too long (85 > 79 characters)

Line 63:80: E501 line too long (82 > 79 characters)
Line 74:80: E501 line too long (86 > 79 characters)
Line 81:80: E501 line too long (83 > 79 characters)
Line 139:80: E501 line too long (86 > 79 characters)
Line 190:80: E501 line too long (85 > 79 characters)
Line 292:80: E501 line too long (82 > 79 characters)
Line 417:80: E501 line too long (85 > 79 characters)
Line 429:80: E501 line too long (81 > 79 characters)
Line 591:80: E501 line too long (80 > 79 characters)
Line 606:80: E501 line too long (81 > 79 characters)
Line 610:80: E501 line too long (81 > 79 characters)
Line 675:80: E501 line too long (88 > 79 characters)
Line 680:80: E501 line too long (84 > 79 characters)
Line 693:80: E501 line too long (84 > 79 characters)
Line 706:80: E501 line too long (84 > 79 characters)
Line 721:80: E501 line too long (84 > 79 characters)
Line 734:80: E501 line too long (84 > 79 characters)
Line 749:80: E501 line too long (84 > 79 characters)
Line 768:80: E501 line too long (84 > 79 characters)
Line 776:80: E501 line too long (82 > 79 characters)
Line 784:80: E501 line too long (83 > 79 characters)
Line 887:80: E501 line too long (87 > 79 characters)
Line 905:23: E231 missing whitespace after ','

Line 1325:80: E501 line too long (85 > 79 characters)
Line 1466:80: E501 line too long (90 > 79 characters)
Line 1636:80: E501 line too long (81 > 79 characters)
Line 1647:80: E501 line too long (86 > 79 characters)
Line 1651:80: E501 line too long (80 > 79 characters)

Comment last updated at 2019-05-23 17:12:24 UTC

@tomwhite
Copy link
Collaborator Author

tomwhite commented Apr 3, 2019

This is failing because black isn't being installed, maybe because the build environment is cached?

@lmcinnes
Copy link
Owner

lmcinnes commented Apr 5, 2019

I'm not really sure -- I have never been that good at this CI stuff. I'll try to look into it when I get some time; I'm kind of swamped with things right now.

@jamesmyatt
Copy link
Contributor

jamesmyatt commented May 16, 2019

If you want to use black (which you should), then you should relax the line limit to 88: https://github.com/python/black#line-length.

@jamesmyatt
Copy link
Contributor

Try adding "black" to the dependencies in

conda create -n testenv --yes python=$PYTHON_VERSION pip nose \

@lmcinnes
Copy link
Owner

Thanks @jamesmyatt. I'll see if I can get that sorted soon.

@tomwhite
Copy link
Collaborator Author

Thanks for the suggestion @jamesmyatt. It's getting a bit further now, but is failing one build as black isn't available on Python 2, and another build because the build environment is different in some way. This will need a bit more work...

@lmcinnes
Copy link
Owner

We should just start ignoring python2. It is going to be end of life soon, and I have no particular desire to support it (and have pledged to drop support for it by Jan 1 2020 anyway, so we certainly do not ahve to provide any support after that).

@jlmelville
Copy link
Collaborator

The black test only needs to be carried out on one of the matrix entries, so one solution is only carrying it out on the coverage entry: it's already got its own branch of the bash script so there's no extra complexity added.

If you go this way, I also suggest doing the black check before the tests, rather than after. This way the check fails faster. Also the coverage checks often falsely fail the build due to negligible negative changes in coverage which would otherwise hide a "true" failure due to a forgotten reformatting.

Finally, I added a section about using black before submitting a PR to https://github.com/lmcinnes/pynndescent/blob/master/CONTRIBUTING.md -- it would probably be helpful to add that to this PR too (everything else in the pynndescent document is copy-and-pasted from UMAP, so this is a trivial change).

@tomwhite
Copy link
Collaborator Author

Thanks for the suggestions James. Here's another attempt.

@coveralls
Copy link

coveralls commented May 23, 2019

Coverage Status

Coverage remained the same at 94.544% when pulling 8fb6181 on tomwhite:enforce-black into 43ef61a on lmcinnes:master.

@lmcinnes
Copy link
Owner

Sorry for being slow to get to this, but thanks for all the work people have put it. It is definitely appreciated. Merging now, and thanks again!

@lmcinnes lmcinnes merged commit c61cdfe into lmcinnes:master Jun 23, 2019
@tomwhite tomwhite deleted the enforce-black branch June 26, 2019 12:49
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.

6 participants