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

lint: isort pre-commit & misc improvements #3749

Merged
merged 10 commits into from
May 6, 2020

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented May 6, 2020

@casperdcl casperdcl requested a review from efiop May 6, 2020 13:17
@casperdcl casperdcl self-assigned this May 6, 2020
@casperdcl casperdcl added enhancement Enhances DVC refactoring Factoring and re-factoring research testing Related to the tests and the testing infrastructure and removed enhancement Enhances DVC labels May 6, 2020
@casperdcl casperdcl changed the title tests: pre-commit: isort lint: isort pre-commit & misc improvements May 6, 2020
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to have this long time ago. Thanks @casperdcl 🎉

I have a question though: as this is run in virtualenv created by pre-commit, how'll this find the difference between third party and dvc?

@casperdcl
Copy link
Contributor Author

good point @skshetry - just added a commit

@casperdcl
Copy link
Contributor Author

not sure why travis fails... Works fine locally:

$ git clone --single-branch --branch test-isort casperdcl/dvc
Cloning into 'dvc'...
done.
$ cd dvc
dvc (test-isort)$ pre-commit run --all-files
black....................................................................Passed
isort....................................................................Passed

@skshetry
Copy link
Member

skshetry commented May 6, 2020

It's because of dvc's pre-commit hook. Not sure, why it's broken. /cc @efiop

@efiop
Copy link
Contributor

efiop commented May 6, 2020

@skshetry Not because of dvc hook at all, you can see isort pre-commit hook failing. It reformats the code. Not sure what's up. Maybe sort version or something.

@efiop
Copy link
Contributor

efiop commented May 6, 2020

@casperdcl Probably should also add isort to restyled config.

@casperdcl
Copy link
Contributor Author

casperdcl commented May 6, 2020

It's because of dvc's pre-commit hook. Not sure, why it's broken.

No, that only fails locally on mutli-core machines where the diff touches more files than there are hyperthreads. Adding require_serial: true in the pre-commit config doesn't seem to help. :(

Not because of dvc hook at all, you can see isort pre-commit hook failing. It reformats the code. Not sure what's up.

exactly

Maybe sort version or something.

I pinned it though (to 4.3.21)

Probably should also add isort to restyled config.

will do.

@casperdcl
Copy link
Contributor Author

casperdcl commented May 6, 2020

well @efiop's restyled suggestion + @skshetry's note regarding third party equals #3750 - specifically, ea7c52fe192f31df4f3a273143c2d92a61024f12 - clearly not picking up the config for dvc being a first-party lib. Maybe this is because the config isn't in master yet?

@efiop
Copy link
Contributor

efiop commented May 6, 2020

@casperdcl Yeah, sounds likely. Could merge as is (even if it fails) and deal with it on top.

@casperdcl
Copy link
Contributor Author

@efiop based on https://github.com/iterative/dvc/runs/650806804 this is good to merge

@efiop efiop merged commit 2bb7257 into iterative:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Factoring and re-factoring research testing Related to the tests and the testing infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants