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.

Sign up for GitHub

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

Jump to bottom

Style: enforce import ordering with isort (YTEP0037 1/6) #2592

Merged
merged 7 commits into from
Jul 16, 2020
Merged

Style: enforce import ordering with isort (YTEP0037 1/6) #2592

merged 7 commits into from
Jul 16, 2020

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented May 15, 2020 •
edited
Loading

PR Summary

isort is a tool that automatically sorts import statements.
It can be configured as a precommit hook too, so it should not be necessary to have this kind of PR, updating the whole code base more than once.

Imo, the 4.0 release would be a nice time to include this kind of nitpicky change, though I can easily imagine how the present PR would clog the yt-4.0 -> master merge, so I wouldn't suggest considering it for a merge ahead of this major task.

CURRENT STATE: This PR should include everything needed to smoothly adopt isort, but doesn't run it on the code base as it used to.

checklist

Validation steps

  • ensure compatibility with flake8 (noqa lines)
  • add minimal black-compatible isort configuration to setup.cfg
  • check commutativity with black
  • identify and declare files to be ignored by isort
  • polish isort configuration : declare unyt as second or third party depending on the outcome of YTEP0037's vote

Documentation and workflow

@neutrinoceros neutrinoceros added proposal Proposals for enhancements, changes, etc code style Related to linting tools labels May 15, 2020
@neutrinoceros neutrinoceros marked this pull request as draft May 15, 2020 19:46
@neutrinoceros neutrinoceros added the enhancement Making something better label May 15, 2020
@matthewturk
Copy link
Member

matthewturk commented May 15, 2020 via email

@neutrinoceros
Copy link
Member Author

Actually I was reading through pandas' issue tracker to try and discover how they managed their transition to black on such a large project. Couldn't find the answer yet but doing so I just learned about isort, which seemed like a first step, much more lightweighted rewiew-wise.

So yeah, I would also like to see black adopted, but I can certainly understand the anxiety ! In any case I don't want to get in the way of #2437

@matthewturk
Copy link
Member

matthewturk commented May 15, 2020 via email

@neutrinoceros
Copy link
Member Author

Just a quick note: I just tried to combo isort + black on a personal project and they seem to have minor disagreements over formatting of comma-separated multiple-lines imports.
The conclusion is that it's definitely a good idea to discuss them together, but there's some tweaking needed to keep them playing nicely with each other.

@neutrinoceros
Copy link
Member Author

Here's the pandas thread pandas-dev/pandas#26926
including a solution to the isort + black issue :)

@neutrinoceros neutrinoceros force-pushed the isort_sort_imports branch 2 times, most recently from e5ee991 to 022d95e Compare May 16, 2020 10:50
@neutrinoceros
Copy link
Member Author

Apparently doing the isort pass on the whole code base at once breaks the installation (pip install -e . still runs but then the lib is unusable). Going to try a more iterative approach to identify what's going wrong.

@neutrinoceros neutrinoceros force-pushed the isort_sort_imports branch 2 times, most recently from c1d6ba5 to 48b7faf Compare May 16, 2020 12:38
@neutrinoceros
Copy link
Member Author

neutrinoceros commented May 16, 2020 •
edited
Loading

After a few iterations and rebasing, I think I've covered every little problem caused by isort-ing the code base. I also made sure to leave it in a state such that (black + isort) would be a commutative combo (only one change was needed for this).

There remain 2 open questions:

  • should I bundle in a precommit hook ? This would guarantee future contributions keep the code isort-ed, but it forces developers to install isort (not an issue imo, as long as it's properly documented)
  • more generally, and to anticipate a later adoption of black, what line-length should we settle for ? isort also has such an argument; I currently set it to 100, which is larger than black's default 88.

@neutrinoceros neutrinoceros force-pushed the isort_sort_imports branch 3 times, most recently from 1d8346c to 8946ed0 Compare May 16, 2020 14:50
@neutrinoceros
Copy link
Member Author

Interestingly, isort gives slightly different results in python 3.6 on travis VS 3.8 on my machine.
I think that fixing this just requires a bit of tweaking in setup.cfg

@neutrinoceros neutrinoceros force-pushed the isort_sort_imports branch 5 times, most recently from 3c7c1c9 to 2621ba1 Compare May 16, 2020 18:00
@neutrinoceros neutrinoceros marked this pull request as ready for review May 16, 2020 19:03
@neutrinoceros neutrinoceros changed the title proof of concept: run isort on the whole codebase Styling: enforce import ordering with isort May 16, 2020
@neutrinoceros neutrinoceros changed the title Styling: enforce import ordering with isort Style: enforce import ordering with isort May 16, 2020
This was referenced May 16, 2020
Merged
Closed
@neutrinoceros neutrinoceros mentioned this pull request May 19, 2020
Merged
@neutrinoceros neutrinoceros added the yt-4.0 feature targeted for the yt-4.0 release label Jun 11, 2020
@neutrinoceros
Copy link
Member Author

This should not go in before #2640 and #2641

@munkm munkm changed the base branch from yt-4.0 to master June 22, 2020 15:17
@neutrinoceros neutrinoceros changed the title Style: enforce import ordering with isort Style: enforce import ordering with isort (YTEP0037 1/6) Jun 22, 2020
@neutrinoceros neutrinoceros force-pushed the isort_sort_imports branch 3 times, most recently from 76abfcd to 2ac9c58 Compare June 23, 2020 21:31
@neutrinoceros
Copy link
Member Author

Conflicts will show up every time someone changes/adds an import statement in master. I'll wait for this PR to be prioritised to solve this in one clean rebase.

@neutrinoceros neutrinoceros force-pushed the isort_sort_imports branch 4 times, most recently from 77cb421 to f2eb8a3 Compare July 2, 2020 07:04
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Jul 2, 2020 •
edited
Loading

quick sanitising of this branch:

  • rebased onto master
  • put the "isort run" commit at the end of the history so it's easier to remove and rerun for future rebasing
  • document line-length 88 and "unyt is third party"

actually I'm just going to remove the final commit. That will also facilitate the reviewing process

@brittonsmith brittonsmith merged commit 2e08209 into yt-project:master Jul 16, 2020
@neutrinoceros neutrinoceros mentioned this pull request Jul 16, 2020
Merged
@neutrinoceros neutrinoceros deleted the isort_sort_imports branch July 21, 2020 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chummels chummels chummels approved these changes

@munkm munkm munkm approved these changes

@matthewturk matthewturk matthewturk approved these changes

Assignees
No one assigned
Labels
code style Related to linting tools enhancement Making something better proposal Proposals for enhancements, changes, etc yt-4.0 feature targeted for the yt-4.0 release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@neutrinoceros @matthewturk @munkm @chummels @brittonsmith