Use isort with pre-commit to enforce import guidelines #5659
Conversation
There was a problem hiding this comment.
This seems nice!
One import issue that it reminded me of is whether we should be putting the entire path in our import statements. For example from networkx.algorithms.approximation import ... when it could be from networkx.approximation import .... The longer way shows users where to find the code, while the shorter form is presumably what we expect them to use e.g. with tab-completion while typing code. Since I can see either way being reasonable, it is best to pick one and automate that shift. Maybe that is already what this (or even the current code-base) does. :}
I'll look through all these changes sometime soon.
There was a problem hiding this comment.
I've gone through all the changes and they look fine to me -- teher are a lot of them, but they are all basically the same kind of changes.
It was clear to me that the fairly recent imports of subpackages allow us to get rid of some of the import statements. We can now use @nx.utils.not_implemented_for for example instead of an explicit import. But I'm not which is better and that issue is separate from this.
It looks like it is set to avoid polluting the Blame Space. So it looks good to me.
There was a problem hiding this comment.
Of all the auto-linters/formatters I've come across, I personally find isort the most nitpicky/least useful... that said, I agree w/ @MridulS : just do it and you never have to think about it again!
AIUI isort is indeed a pretty standard linter these days. I know we've had PRs in the past where changes have been made to imports in otherwise untouched files, presumably because the contributors' IDEs have isort built in. Since it's a standard tool and integrates seamlessly into the developer workflow via pre-commit, I'm +1.
I tested the pre-commit hooks locally and everything worked as expected. Since we squash-merge everything by default, won't the commit hash in the .git-blame-ignore-revs have to be changed after merge? Just a minor detail to follow-up on!
It will indeed, I just put that in for testing locally. I will change the hash after the PR is merged :) |
|
Pulled in the main branch and reran this to fix merge conflicts. |
We should make a decision on this ASAP or else you're going to be doing a lot of rebasing 😂 |
* Add isort to pre-commit * Run isort on all python files (except __init__.py ones)
Another precommit automation where we can stop thinking about what and how to do things.
This does not touch any of the
__init__.pyfiles.(I thought of doing this after reading #2723)