search

mintel / build-harness

🤖Collection of Makefiles to facilitate building Python/Golang projects, Dockerfiles, and more
Apache License 2.0
2 stars 6 forks source link

Should isort be included as default lint target? #29

Open xiang-zhu opened 4 years ago

xiang-zhu commented 4 years ago

isort seems having compatibility issues with black https://github.com/psf/black/issues/333 https://github.com/timothycrosley/isort/issues/694

There's a certain config to work around the issue, and implemented here through https://github.com/mintel/build-harness/pull/27

I want to start this as dicussion of is isort provide enough value that we want to take on the maintenance responsibility for the incapability and potential breaking change in the future?

charlieparkes commented 4 years ago

The whole point of build harness is that we control for potential breaking changes by centralizing all the commands we run in one place. If isort+black end up with a serious issue again in the future (issue #27 was not serious), we can remove isort or adjust the commands as necessary.

That said, I think there's potential to drop isort from the lint target (leave it in the fmt target) with the addition of flake8. I'll investigate #28 before returning to this issue.

charlieparkes commented 4 years ago

Ok, so #31 set the following order-of-operations.

Lint

  1. autoflake - delete unused imports and variables, if possible
  2. isort - clean up imports (ordering, formatting)
  3. black - "no opinions allowed, this is the way"
  4. flake8 - list style and quality errors that impact readability and/or are the source of bugs

All of these things blend together well and do something the others don't. I limited each to do the bare minimum, where possible, so there's very little duplicate work. (isort and black may try to duplicate each-other, but it shouldn't cause any conflicts in the current configuration)