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

python: reformat and addition of format checks #1802

Merged
merged 6 commits into from Dec 28, 2018

Conversation

Projects
None yet
5 participants
@trws
Copy link
Member

trws commented Nov 5, 2018

Currently this applies the "black" formatter to our python code globally and adds two scripts, one that runs it to format all python code int the tree and another that checks whether formatting would change the code. I'm planning to have the check script run in at least one of the travis targets, probably along with pylint.

The question now, is should I also add the clang-format formatting for our C code to this. It's not materially harder, but before I do it, we need to pick a standard version to use (I suggest 6.0 or 7.0) and agree that the format file as defined in this PR (same as we had but disregarding support for 3.6, which is now ancient) is what we want code to look like or modify it such that it is. Thoughts?

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Nov 6, 2018

Hilariously, IMO, the formatter used by pylint generates code that fails pylint. I will adjust our settings so the checks match the formatting.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #1802 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1802      +/-   ##
==========================================
- Coverage   80.12%   80.11%   -0.02%     
==========================================
  Files         196      196              
  Lines       35064    35064              
==========================================
- Hits        28096    28092       -4     
- Misses       6968     6972       +4
Impacted Files Coverage Δ
src/modules/connector-local/local.c 73.77% <0%> (-1.04%) ⬇️
src/common/libflux/message.c 81.64% <0%> (+0.36%) ⬆️
@SteVwonder
Copy link
Member

SteVwonder left a comment

Thanks for putting this together, @trws!

Can we add check-format as one of the MAKECMDS in travis_run.sh so that Travis will turn red if the formatting is incorrect? A more extreme option would be to create a separate test within the travis build matrix, like Spack has.

@@ -44,7 +44,7 @@ symbols=no
# --enable=similarities". If you want to run only the classes checker, but have
# no Warning level messages displayed, use"--disable=all --enable=classes
# --disable=W"
disable=missing-docstring, too-few-public-methods, too-many-arguments, star-args, arguments-differ, no-member, import-error, no-name-in-module, useless-object-inheritance, fixme
disable=missing-docstring, too-few-public-methods, too-many-arguments, star-args, arguments-differ, no-member, import-error, no-name-in-module, useless-object-inheritance, fixme, C0330

This comment has been minimized.

@SteVwonder

SteVwonder Nov 6, 2018

Member

Do you happen to know what the "human-readable" code for C0330 is? If so, can we use that instead? Based on this issue, I believe it is bad-continuation.

This comment has been minimized.

@trws

trws Nov 7, 2018

Author Member

I'll try that, good point.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Nov 7, 2018

Definitely, I want this check to be part of travis, and I want this and pylint to run first rather than last as is being done now. There's little more frustrating than compiling, checking and passing everything only to run the format check and fail at the end. 😖

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 6, 2018

The question now, is should I also add the clang-format formatting for our C code to this.

I think we agreed that C should be a separate PR/issue.

In addition to this, we should probably also add a python section to RFC 7 to document our practices/expectations for people submitting PR's to framework projects. Sounds like it can just reference the the Black code style instead of the specific details, if we are happy with that style.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 10, 2018

I have a locally rebased version of this waiting on #1795, having outstanding python PRs during a reformat would be unfortunate, should be very quick once the other merges.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 21, 2018

@trws, I'm adding this to the 0.11.0 release milestone per your comments yesterday.

@garlick garlick added this to the release 0.11.0 milestone Dec 21, 2018

@garlick garlick changed the title reformat and addition of format checks python: reformat and addition of format checks Dec 21, 2018

@trws trws force-pushed the trws:format-checks branch 4 times, most recently from 1dbafa1 to ed1bae7 Dec 21, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 21, 2018

Ok, this includes formatting of all the python code, rebased on current master, with a new build stage setup in travis so that the main builds wont even try to run unless the style checks pass (faster feedback for the thing that's quick to check, less wasted build cycles, etc.). Assuming the currently running travis build passes, this should be GTG.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 21, 2018

Travis passed, but apparently codecov/project did not. I'm not sure what to do about that given that this PR didn't actually add any new code (except the check script). Perhaps it doesn't like that the code is now on more lines?

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Dec 23, 2018

Thanks @trws!

Other than the pylintrc comment above and some commit polishing, this LGTM! I wouldn't worry about the codecov since it doesn't track python files.

Per the commit polishing, before merging, it would be good to squash some of the incremental commits and reword the commit titles to the form topic: description.

@trws trws force-pushed the trws:format-checks branch from ed1bae7 to a8ed2ae Dec 27, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 27, 2018

Thanks @SteVwonder, the commits have been re-worked, and the bad-continuation option seems to work. Assuming I didn't muck something else up and the rest of the travis run passes, we should be GTG.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 27, 2018

I think that last commit could be squashed with the first one.

Making this a separate stage in travis looks really nice in the ci summary page! @grondo could you give your ack on the travis changes?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 27, 2018

Oh yeah, Travis changes look great. Thanks, @trws!

@trws trws force-pushed the trws:format-checks branch from a8ed2ae to b42a4cb Dec 28, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 28, 2018

Good point, squashed and re-pushed.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 28, 2018

Restarted a builder that failed the wreck epilog test.

Apologies @trws, this needs one more rebase then I'll press the button.

@trws trws force-pushed the trws:format-checks branch from b42a4cb to ab5dfd8 Dec 28, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 28, 2018

Thanks!

@garlick garlick merged commit d59ce0a into flux-framework:master Dec 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.