ethpm / py-ethpm

This library is deprecated. ethPM python tooling is now located in web3.py
MIT License
24 stars 13 forks source link

Isort #52

Closed njgheorghita closed 6 years ago

njgheorghita commented 6 years ago

What was wrong?

Import sorting was too subjective.

How was it fixed?

Use isort library and config's from web3 to sort import statements

Cute Animal Picture

image

davesque commented 6 years ago

This looks good. Did you by chance use the isort config from the project template?

njgheorghita commented 6 years ago

@davesque Grabbed it from web3 - but looks to be just the same as the template. The only difference is that in web3, pytest is listed as a known_standard_library and in the template it's listed as a known_third_party, i'm thinking the template is correct, right?

njgheorghita commented 6 years ago

Acutally, i'm not too sure - I'm starting to think i like sticking pytest in with the standard lib import statements

davesque commented 6 years ago

The template is correct. They had been doing that since pytest is so commonly used as to be almost like a standard library for us. However, I suggested that this is really confusing to newcomers and it violates the principle of least astonishment. So we updated the template to mark pytest as a known third party module, which it is.

njgheorghita commented 6 years ago

@davesque good to know - i'll change it to known_third_party

davesque commented 6 years ago

In general, it's good to look to the template whenever you're manually importing configuration stuff like that. That will hopefully allow us to avoid inconsistent configs. I sort of wish we had an official python project template actually instead of Jason's repo.

davesque commented 6 years ago

Also, don't forget to re-run isort since it's going to change where that module shows up :).

davesque commented 6 years ago

Schweet. Looks like you rebuilt things?

njgheorghita commented 6 years ago

Haha - yeah - and a typo in tox.ini had me tearing out my hair (or what's left of it) for a bit - but everything should be set now

davesque commented 6 years ago

Can you merge or do you need me to?

njgheorghita commented 6 years ago

I got it!