style: isort #519

henryiii · 2021-01-05T22:56:24Z

This actually can do full Black-style includes, I like it. :) Discussed in #518.

style: isort
style: run isort

This is Black-style, which I like - short includes are bundled onto one line, longer ones are one-per line. You can get merge conflicts for the short includes, but hopefully they are not changed often? And matching changes are okay.

Sticking configs in setup.cfg instead of pyproject.toml because mypy and flake8 don't support that yet. (Flake8 may never :( )

henryiii · 2021-01-05T23:02:55Z

Second commit is completely generated so this can easily be rebased, no rush.

YannickJadoul · 2021-01-05T23:39:42Z

This might be the first time I more or less agree with Black :-o

Two things that're ugly, IMO:

from . import test_projects, utils is importing 2 modules on 1 line; why?
The multiline from ... import (...) is ugly, but arguably not much more ugly than writing less lines with several things on one line.
Oh, yeah, in any test file, having pytest first makes perfect sense to me. But that's personal taste, I guess :-)

I'll come back complaining when Black starts correcting me when I'm sure I'm right ;-)

Czaki · 2021-01-05T23:45:42Z

https://pycqa.github.io/isort/

What is wrong with grouping imports?

henryiii · 2021-01-06T00:47:43Z

The multiline from ... import (...) is ugly

Sorting imports and many of the decisions in Black are not about beauty, but are about reducing merge conflicts. Sorted imports would have removed 1 of 6 merge conflicts when I rebased #511, Black would have removed at least 2 more with function arguments. My work would have been cut in half.

Might be a way to turn that off? Weird. We can try to fix it. There are lots of options, unlike Black. ;)
Multiline imports are great; if I add Set and you add Union, we don't conflict! Same thing for function arguments if there are more than a couple or so.
PEP 8 grouping says stdlib first, 3rd party next, internal last. That's nice to help show what is stdlib vs. added, and that can actually catch bugs, if you import X and think that it's local (Python 3) or from the stdlib/remote (Python 2), it will try to move it and you'll see the mistake. I had to fix someone who was using import cmake for a local file when they also had pip installed cmake.

PS: TLDR; 3 doesn't bother me that much, I'd rather consistency. But it might even be controllable.

henryiii · 2021-01-06T01:11:28Z

You could put pytest first: https://pycqa.github.io/isort/#custom-sections-and-ordering :)

henryiii · 2021-01-06T01:25:15Z

test/test_before_all.py

-from . import test_projects
+import pytest
+
+from . import test_projects, utils


This is slightly confusing, actually, the thing after a from x import ideally shouldn't be a module; if you have an __init__.py and it contains a test_projects or utils object, you'll get that instead of the module. But it's a bit special for . imports, since you are required to use from with a relative import.

I believe you are guaranteed to have the test directory as the first on the path in PyTest, so this could be just import test_projects, etc? Also, we could from .test_projects import new_c_project, etc.

Some of the exiting ones are written this way, so I'd probably leave it for now, and eventually rewrite them properly (see test_0_basic.py, for example).

Yeah, I first wanted to suggest import .test_projects, but apparently that only works in the from ... import ... construct.

the thing after a from x import ideally shouldn't be a module

Not sure I agree, e.g. from numpy import linalg from django import models from tensorflow import keras... also I'd rather rely on the __init__.py file being empty than rely on the value of sys.path. I expect tools like mypy or language servers to be able to read the code better without relying on a sys.path set by pytest too.

from numpy import linalg is an often used shortcut for import numpy.linalg as linalg, but it is confusing - a reader can't tell if linalg is a package or an object. We are stuck with it, but I wish the language wasn't ambiguous. Ideally, relative imports probably should import objects, from .test_projects import new_c_project.

Relative imports also don't work if you don't execute it as a package; python test_before_all.py will fail. (ImportError: attempted relative import with no known parent package) (PyTest handles this correctly).

I'm not sure it matters, from a users' point of view.

If you try to rewrite it, you have to either run it interactively or look up the source. For example:

from a import b b.c() # May or may not be written as from a.b import c c()

And, if you need to look up the source, is there an a/b.py or an a/__init__.py file with a b in it, or is it a file? (I often teach Python, this sort of thing can be a pain point...)

I use relative imports a lot, but they have downsides, especially for new users who do not know how to set up a proper environment. They have to be in a loaded package; it's not only "in the same directory". So python x.py inside a package fails if there are relative imports in x; I think python outer/x.py fails too (I could be wrong). If you stick a if __name__ == "__main__" in it for standalone runs or tests, it fails.

I also use absolute imports a lot...

I do know someone who will not do any import except import a.b.c, and always uses fully qualified names. That's it. No shortening. It drive me crazy seeing numpy.stuff sometimes. :)

FWIW (given we're on this tangent anyway?), I tend to not like from module import thing either. Often, module.thing is not so much longer to write, should be auto-completed, and we're not trying to save bytes in code anyway? But yes, numpy to np just becomes a habit, after a while :-)

henryiii · 2021-01-06T01:31:40Z

unit_test/main_tests/conftest.py

-    util,
-    windows,
-)
+from cibuildwheel import linux, macos, util, windows


This is irritating, it didn't respect Black's magic comma.

I guess it doesn't. Sad. For things like typing I add the comma because I know they are likely to change. (Not true of this case, of course, but might be useful elsewhere, especially with typing).

This is irritating, it didn't respect Black's magic comma.

What did you mean? Black put , only at the end of the multiline import. When the import is a single line then black does not put a comma on the end.

True, that would be nice, if we could teach it. And once again (I'm surprised) I would agree with Black that the comma there is a pretty good indicator to say that this is a changing list.

YannickJadoul · 2021-01-06T12:18:04Z

Yeah, don't pay too much attention to my grumbling. These are just the things that I'm used to, so not even sure whether this was cibuildwheels's (implicit) style.
I must say I'm surprised by the merge conflicts. I don't think I ever ran into those too much for includes. Or maybe they are so easily fixed that I just forgot about them :-D

Czaki · 2021-01-06T12:21:10Z

I must say I'm surprised by the merge conflicts. I don't think I ever ran into those too much for includes. Or maybe they are so easily fixed that I just forgot about them :-D

After finish the discussion here the simplest thing will be created the next PR which starts from the current master and has copied isort configuration.

joerick

Looks great!

joerick · 2021-01-06T17:28:29Z

test/test_before_all.py

-from . import test_projects
+import pytest
+
+from . import test_projects, utils


the thing after a from x import ideally shouldn't be a module

Not sure I agree, e.g. from numpy import linalg from django import models from tensorflow import keras... also I'd rather rely on the __init__.py file being empty than rely on the value of sys.path. I expect tools like mypy or language servers to be able to read the code better without relying on a sys.path set by pytest too.

henryiii · 2021-01-06T18:54:11Z

To put @Czaki's recommendation into code; if you are working on a branch:

git checkout master -- .pre-commit-config.yaml setup.cfg
pre-commit run -a
git add -u
git commit -m "style: pre-commit run -a"

Then you can squash on master if you want. Or git commit --amend, etc.

henryiii · 2021-01-06T18:55:15Z

Also, for simplicity, I'll rebase and merge instead of squash and merge. Maybe we could even eventually add a .git-blame-ignore-revs file and add these style change runs to it.

henryiii · 2021-01-06T20:06:37Z

Permissions error on Windows Travis can be ignored, I think.

YannickJadoul · 2021-01-06T21:04:36Z

Permissions error on Windows Travis can be ignored, I think.

I restarted that build, but indeed. Go ahead if you want to merge :-)
Wait, I'll still give you that precious checkmark.

YannickJadoul

(Unless @joerick still wants to fix the from . import x, y, but you already got an approval there, so I'm not holding up this PR on that!)

henryiii · 2021-01-06T23:10:52Z

There already was a from . import x, y in the codebase, so not important, I think. ;)

henryiii · 2021-01-06T23:22:39Z

Actually, even easier, just rebase, delete the merge conflict lines, and then run pre-commit run -a, then git rebase --continue. Done. :)

henryiii commented Jan 6, 2021

View reviewed changes

joerick approved these changes Jan 6, 2021

View reviewed changes

henryiii added 2 commits January 6, 2021 13:50

style: isort

538c068

style: pre-commit run -a

749201a

henryiii force-pushed the style/isort branch from a0f2d62 to 749201a Compare January 6, 2021 18:51

YannickJadoul approved these changes Jan 6, 2021

View reviewed changes

henryiii merged commit 635b226 into pypa:master Jan 6, 2021

henryiii deleted the style/isort branch January 6, 2021 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: isort #519

style: isort #519

henryiii commented Jan 5, 2021 •

edited

Loading

henryiii commented Jan 5, 2021

YannickJadoul commented Jan 5, 2021

Czaki commented Jan 5, 2021

henryiii commented Jan 6, 2021 •

edited

Loading

henryiii commented Jan 6, 2021

henryiii Jan 6, 2021

henryiii Jan 6, 2021

YannickJadoul Jan 6, 2021 •

edited

Loading

joerick Jan 6, 2021

henryiii Jan 6, 2021

henryiii Jan 6, 2021 •

edited

Loading

henryiii Jan 6, 2021

henryiii Jan 6, 2021

henryiii Jan 6, 2021

YannickJadoul Jan 6, 2021

henryiii Jan 6, 2021

henryiii Jan 6, 2021

Czaki Jan 6, 2021

YannickJadoul Jan 6, 2021

YannickJadoul commented Jan 6, 2021

Czaki commented Jan 6, 2021

joerick left a comment

joerick Jan 6, 2021

henryiii commented Jan 6, 2021 •

edited

Loading

henryiii commented Jan 6, 2021

henryiii commented Jan 6, 2021

YannickJadoul commented Jan 6, 2021

YannickJadoul left a comment

henryiii commented Jan 6, 2021

henryiii commented Jan 6, 2021 •

edited

Loading

style: isort #519

style: isort #519

Conversation

henryiii commented Jan 5, 2021 • edited Loading

henryiii commented Jan 5, 2021

YannickJadoul commented Jan 5, 2021

Czaki commented Jan 5, 2021

henryiii commented Jan 6, 2021 • edited Loading

henryiii commented Jan 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YannickJadoul Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YannickJadoul commented Jan 6, 2021

Czaki commented Jan 6, 2021

joerick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented Jan 6, 2021 • edited Loading

henryiii commented Jan 6, 2021

henryiii commented Jan 6, 2021

YannickJadoul commented Jan 6, 2021

YannickJadoul left a comment

Choose a reason for hiding this comment

henryiii commented Jan 6, 2021

henryiii commented Jan 6, 2021 • edited Loading

henryiii commented Jan 5, 2021 •

edited

Loading

henryiii commented Jan 6, 2021 •

edited

Loading

YannickJadoul Jan 6, 2021 •

edited

Loading

henryiii Jan 6, 2021 •

edited

Loading

henryiii commented Jan 6, 2021 •

edited

Loading

henryiii commented Jan 6, 2021 •

edited

Loading