-
Notifications
You must be signed in to change notification settings - Fork 94
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
CI: cleanup style check #477
Conversation
b878894
to
8ec1e9e
Compare
f463832
to
df1c0fd
Compare
df1c0fd
to
02c5f49
Compare
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #477 +/- ##
===============================================
+ Coverage 90.23% 90.40% +0.17%
===============================================
Files 15 15
Lines 1126 1126
===============================================
+ Hits 1016 1018 +2
+ Misses 110 108 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @madsbk
Thanks Mads! 😄 Tried to fix this a while back ( #350 ), but ran into issues 😅 Guess the linter tools were too old |
hooks: | ||
- id: black | ||
- repo: https://gitlab.com/pycqa/flake8 | ||
rev: 3.7.7 | ||
rev: 3.8.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review here, but why did we pin those versions specifically instead of those from the integration repo (see https://github.com/rapidsai/integration/blob/branch-0.18/conda/recipes/versions.yaml) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of these tools weren't respecting line length configuration values in some cases. Ran into similar issues as well. Though this does a raise a good point, would be good to propagate these version bumps into the integration repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideas on what's the right approach? Pinning those exact versions in integration will probably lead all other RAPIDS repos to require some reformatting.
This PR clean up the style check output by capturing both stdout and stderr.
Also
isort
line-length is now 88, which is whatblack
andflake8
use.