-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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: remove duplicated code check #36642 #36716
CI: remove duplicated code check #36642 #36716
Conversation
fangchenli
commented
Sep 28, 2020
•
edited
Loading
edited
- closes CI: Deduplicate linting checks #36642
I wonder if it's also possible to move flake8-rst into pre-commit, perhaps as a local hook. Maybe @MarcoGorelli knows? |
xref #34150 flake8-rst is not under active development anymore. It requires flake8<3.8.0 |
Is flake8-rst the only thing blocking us from upgrading flake8? If so, I'd say let's fork it, run it in pre-commit (so it's in its own environment), and no longer pin flake8. Then later we can replace it with a different check (if necessary), but for now this would ensure the same checks will be run EDITtaken forward in #36722 |
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.
Looks good. Comment on potential additional cleanup otherwise lgtm
@simonjayhawkins I think should back port? |
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.
lgtm
Sorry to backtrack, but TBH I'd appreciate if we could wait a little before merging this. I noticed in #36978 that isort, when run in pre-commit, wasn't picking up *this is because isort's |
I think better to keep the backport without the newer style checks. If we release 1.2 in November, 1.1.4 maybe the last in the 1.1.x Series. so maybe not that many more regressions to backport now anyhow. |
Hi @fangchenli We now have two separate hooks for isort:
so IMO it's OK to remove it from |
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.
Nice!
Can you merge master to get CI green? |
…i/pandas into remove-duplicate-check
thanks @fangchenli |