Add pre-commit, apply to all files #7641
Conversation
There was a problem hiding this comment.
-
Can you make that two separate commits, one for real changes (
.pre-commit-config.yaml, travis, makefile, docs etc.) and one for changes done automatically byreorder-python-importscommand?This will help during review (you can set github to show changes from one commit and focus on what really matters), and in future when someone will need to go through history for any reason (as automatic changes are clearly separated and easy to ignore).
-
travis failed for this PR in
pre-commit runstep.
mshriver
force-pushed
the
add-pre-commit
branch
from
9cd2a1f
to
9e93325
Compare
mshriver
force-pushed
the
add-pre-commit
branch
from
9e93325
to
a93256a
Compare
There was a problem hiding this comment.
Thanks for splitting commits, that makes it easier to review and read in future.
When I opened travis in the morning, it failed in unrelated robottelo unit test. This is intermittent failure that appears rarely (maybe 5% of PRs top), and usually it's enough to restart build. I did so, but #7625 got merged in the meantime and now causes travis to complain again.
ACK. Obviously we need to rebase before merge.
To anyone who will be merging this: please do not squash the commits :) .
| @@ -348,7 +348,8 @@ you have `graphviz`_ installed:: | |||
|
|
|||
| To check for code smells:: | |||
|
|
|||
| $ make lint | |||
| $ pre-commit install-hooks | |||
| $ pre-commit run --all-files | |||
There was a problem hiding this comment.
Technically changes in that file belong to commit 99b0508 , as they weren't done by pre-commit. But that's not a big deal, we can leave it as is.
|
As we are already touching almost every file in this PR why not applying This other PR #7458 also changes a lot of files, for sure will conflict with this one. I am in favour of doing this change also using pre-commit. |
|
@rochacbruno absolutely we'll be using It will take some rebasing, but I'd rather consider using pre-commit first, and then add more checks to pre-commit once we have the team using it. My preference would be to consider them as separate PRs/changes, but I can certainly be convinced otherwise. |
|
@rochacbruno i'll revive the black PR once this is merged, as I can just add black to the pre-commit |
…unctions Update code_standards doc to reflect flake config Use reorder_python_imports, which will split imports into single line direct imports, instead of allowing grouping.
mshriver
force-pushed
the
add-pre-commit
branch
from
a93256a
to
5b00291
Compare
Codecov Report
@@ Coverage Diff @@
## master #7641 +/- ##
==========================================
+ Coverage 67.57% 67.93% +0.35%
==========================================
Files 30 30
Lines 3778 3814 +36
==========================================
+ Hits 2553 2591 +38
+ Misses 1225 1223 -2
Continue to review full report at Codecov.
|
|
@mshriver is same change going to adopt in 6.6.z/6.5.z/6.4.z branches as well ? |
Include python-reorder, flake, and local uuids-fix to start
Remove pylint config, makefile commands for flake/lint/pre-commit
Run pre-commit on all files in travis
gitignore venv/egg directories
Docs should be updated to reflect these changes, if I get agreement on making this switch I'll make those updates.