Skip to content

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
Collaborator

@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
Collaborator

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 treeverse: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