Skip to content

Standardize Python import ordering #2986

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

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

dbutenhof
Copy link
Member

PBENCH-893

The ordering of Python import statements is a frequent topic of discussion during PR reviews. It's often not easy when writing code to determine whether a given module is "standard library" or not, and it's easy to accidentally add a new symbol out of order.

During a recent review, the isort utility was mentioned. I gave it a try but had some difficulty configuring it to be compatible with our black (despite the built-in "black" profile), and gave up.

As an excuse to put off dealing with the necessity of breaking my development VM (and Varshini's dashboard testing), I decided to experiment a bit more to see if we can solve this issue "once and for all" as we've done with source format style in general by adopting black.

I've now got a configuration (defined in our pyproject.toml file) that causes isort to generate output that makes black happy. (This was not trivial, sadly.) I've also replicated the black exclusion list, as that seemed to be appropriate.

I've added isort along with black and flake8 to the lint tests in our build.sh script.

NOTE: see https://pycqa.github.io/isort/docs/configuration/git_hook.html for instructions on adding isort to your git precommit hook. I've integrated the isort extension (and configuration) into my vscode. (Which will be much more useful once we get all modules to a standard baseline.)

isort changes 132 files in the tree, which are included in this PR. This includes obsolete legacy test files, which I decided weren't worth the effort of excluding.

PBENCH-893

The ordering of Python import statements is a frequent topic of discussion
during PR reviews. It's often not easy when writing code to determine whether
a given module is "standard library" or not, and it's easy to accidentally add
a new symbol out of order.

During a recent review, the `isort` utility was mentioned. I gave it a try but
had some difficulty configuring it to be compatible with our black (despite
the built-in "black" profile), and gave up.

As an excuse to put off dealing with the necessity of breaking my development
VM (and Varshini's dashboard testing), I decided to experiment a bit more to
see if we can solve this issue "once and for all" as we've done with source
format style in general by adopting `black`.

I've now got a configuration (defined in our pyproject.toml file) that causes
`isort` to generate output that makes `black` happy. (This was not trivial,
sadly.) I've also replicated the `black` exclusion list, as that seemed to be
appropriate.

I've added `isort` along with `black` and `flake8` to the lint tests in our
`build.sh` script.

NOTE: see https://pycqa.github.io/isort/docs/configuration/git_hook.html for
instructions on adding `isort` to your git precommit hook. I've also been able
to integrate `isort` into my `vscode` configuration.

`isort` changes 132 files in the tree, which are included in this PR. This
includes obsolete legacy test files, which I decided weren't worth the effort
of excluding.
Copy link
Member

@npalaska npalaska left a comment

Choose a reason for hiding this comment

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

This is good, so we can just run isort on the python file after adding new imports. We just have to run isort before we run black.

@portante portante added this to the v0.72 milestone Aug 29, 2022
@dbutenhof dbutenhof merged commit 73f3a36 into distributed-system-analysis:main Aug 29, 2022
@dbutenhof dbutenhof deleted the isort branch August 29, 2022 14:33
@portante
Copy link
Member

See #2988 for pre-commit hook work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants