isort take 2 #2686

jakkdl · 2023-07-05T18:25:16Z

#2538 got abandoned, but that felt in large part because it was just a one-off random big reorg. This adds isort to CI, and it only needed a single isort: skip for it to work for the whole project.

codecov · 2023-07-05T18:30:57Z

Codecov Report

Merging #2686 (d99e0f0) into master (042f035) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2686      +/-   ##
==========================================
- Coverage   98.84%   98.84%   -0.01%     
==========================================
  Files         113      113              
  Lines       16488    16474      -14     
  Branches     3003     3004       +1     
==========================================
- Hits        16297    16283      -14     
  Misses        134      134              
  Partials       57       57

Impacted Files	Coverage Δ
trio/_abc.py	`100.00% <ø> (ø)`
trio/_core/_io_common.py	`100.00% <ø> (ø)`
trio/_core/_ki.py	`100.00% <ø> (ø)`
trio/_highlevel_open_tcp_listeners.py	`100.00% <ø> (ø)`
trio/_highlevel_socket.py	`100.00% <ø> (ø)`
trio/_tests/test_util.py	`100.00% <ø> (ø)`
trio/abc.py	`100.00% <ø> (ø)`
trio/lowlevel.py	`100.00% <ø> (ø)`
trio/__init__.py	`100.00% <100.00%> (ø)`
trio/_channel.py	`100.00% <100.00%> (ø)`
... and 78 more

A5rocks · 2023-07-06T04:47:09Z

Huh, CI failing seems weird. It doesn't seem to be any sort of environment change but it's hard to imagine import re-ordering affecting static tools like that- actually, it isn't that hard to imagine.

I'll guess there's maybe duplicate somethings somewhere...? Or I don't know, maybe just revert certain files and # isort: skip them. Not sure.

I'm also not too sure about how I feel about such a large diff that will probably lead to a ton of merge conflicts! It's probably fine though since I think most open PRs are dead (as far as I know).

jakkdl · 2023-07-06T10:59:19Z

trio/_tests/test_exports.py

-        if tool == "jedi" and sys.version_info >= (3, 11):
-            if class_ in (
-                trio.DTLSChannel,
-                trio.MemoryReceiveChannel,
-                trio.MemorySendChannel,
-                trio.SSLListener,
-                trio.SocketListener,
-            ):
-                missing.remove("__aenter__")
-                missing.remove("__aexit__")
-            if class_ in (trio.DTLSChannel, trio.MemoryReceiveChannel):
-                missing.remove("__aiter__")
-                missing.remove("__anext__")
+        # if tool == "jedi" and sys.version_info >= (3, 11):
+        # if class_ in (
+        #    trio.DTLSChannel,
+        #    trio.MemoryReceiveChannel,
+        #    trio.MemorySendChannel,
+        #    trio.SSLListener,
+        #    trio.SocketListener,
+        # ):
+        #    missing.remove("__aenter__")
+        #    missing.remove("__aexit__")
+        # if class_ in (trio.DTLSChannel, trio.MemoryReceiveChannel):
+        #    missing.remove("__aiter__")
+        #    missing.remove("__anext__")


the 3.11&3.12 failures are because I accidentally pushed this - I think my local environment updated jedi or something and these are no longer missing, but I forgot to undo the commenting before pushing in this PR.

Will look at the windows fails

jakkdl · 2023-07-06T11:12:00Z

trio/socket.py

 if sys.platform != "win32" or not _t.TYPE_CHECKING:
    try:
        from socket import (
-            sethostname as sethostname,
+            if_indextoname as if_indextoname,
            if_nameindex as if_nameindex,
            if_nametoindex as if_nametoindex,
-            if_indextoname as if_indextoname,
+            sethostname as sethostname,
        )
    except ImportError:
        pass


The windows CI fails is because pyright now doesn't see if_indextoname, if_nameindex or if_nametoindex on windows ... but it sees sethostname? And why did it "work" in the first place, since sys.platform == "win32" and _t.TYPE_CHECKING == TRUE it shouldn't have ever seen these symbols if it's parsing the if statement.

Regardless, I don't think it's an issue, and I don't care to dig deeper into it atm, so I'll just Remove those in the test.

jakkdl · 2023-07-06T11:20:36Z

I'm also not too sure about how I feel about such a large diff that will probably lead to a ton of merge conflicts! It's probably fine though since I think most open PRs are dead (as far as I know).

They're either dead, or by me 😇
In the long term this should cut down on merge conflicts from imports though, and hopefully any dead PR's that get resurrected doesn't play around too much with imports, and resolving import conflicts is usually much easier than in normal code (esp when you can just add both sets fully, then run isort+flake8 w/ f401)

jakkdl · 2023-07-06T12:12:38Z

The only failing check is an erroneously duplicated codecov/project

A5rocks

Passes a spot check. I didn't look too hard but I assume this is all good cause tests pass!

I'm going to see if people on Gitter have Opinions about this but I think this is a good enough change to merge despite merge conflicts it will cause.

pyproject.toml

trio/__init__.py

A5rocks · 2023-07-07T00:01:43Z

trio/_core/_generated_instrumentation.py

Do we need to run isort after it's regenerated?

I manually sorted the imports in gen_exports.py so they pass - though upon reflection I should probably just # isort: skip as that's how the black formatting is handled, and shoehorning in isort to work with gen_exports.py --check or forcing future people to manually sort the imports does not sound fun.

Kludex · 2023-07-07T06:59:32Z

I didn't read the PR or issue, but this is useful for this kind of refactor.

jakkdl · 2023-07-07T09:28:38Z

I didn't read the PR or issue, but this is useful for this kind of refactor.

oooh, this is great. Thanks a lot!

Kludex · 2023-07-07T09:36:28Z

I don't think you can add the .git-blame-ignore-revs on this PR, it needs to be on the next one, cause the commits are going to be squashed (I guess).

jakkdl · 2023-07-07T09:38:10Z

I don't think you can add the .git-blame-ignore-revs on this PR, it needs to be on the next one, cause the commits are going to be squashed (I guess).

I rebased and split the PR among three commits, one to add isort to CI and configuring it (that's not ignored by blame, so people can see when it was introduced), one that runs isort on all the files, and a third one that adds .git-blame-ignore-revs. I can then merge/rebase it without squashing and keep all three commits

Kludex · 2023-07-07T09:42:41Z

[...] without squashing and keep all three commits

Great! 👍

jakkdl · 2023-07-07T10:32:09Z

Since nobody's complaining, I'll go ahead and merge 🚀

Kludex · 2023-07-07T10:40:55Z

I think the commit is actually 933f77b ?

jakkdl · 2023-07-07T12:55:16Z

oh huh, yep. It's definitely listed as bec33b2 in this PR, so it must've gotten changed when I rebased it onto master. Will do a quick PR to fix it, thanks!

jakkdl mentioned this pull request Jul 6, 2023

How much runtime introspection of types is a good thing? #2687

Closed

jakkdl commented Jul 6, 2023

View reviewed changes

jakkdl mentioned this pull request Jul 6, 2023

Is it possible to make codecov action not "fail" until all other runs are completed? #2689

Open

jakkdl requested a review from A5rocks July 6, 2023 12:11

A5rocks reviewed Jul 7, 2023

View reviewed changes

jakkdl added 3 commits July 7, 2023 11:34

add isort

af024b3

run isort on all files

bec33b2

add isort commit to .git-blame-ignore-revs

d99e0f0

jakkdl force-pushed the isort branch from 5d7b9c1 to d99e0f0 Compare July 7, 2023 09:35

jakkdl merged commit 755f520 into python-trio:master Jul 7, 2023
33 checks passed

jakkdl deleted the isort branch July 7, 2023 10:32

jakkdl mentioned this pull request Jul 12, 2023

Sort imports #2538

Closed

jakkdl mentioned this pull request Oct 26, 2023

Mark pure reformatting PRs in .git-blame-ignore-revs #2837

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

isort take 2 #2686

isort take 2 #2686

jakkdl commented Jul 5, 2023

codecov bot commented Jul 5, 2023 •

edited

Loading

A5rocks commented Jul 6, 2023

jakkdl Jul 6, 2023 •

edited

Loading

jakkdl Jul 6, 2023

jakkdl commented Jul 6, 2023

jakkdl commented Jul 6, 2023

A5rocks left a comment

A5rocks Jul 7, 2023

jakkdl Jul 7, 2023

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

isort take 2 #2686

isort take 2 #2686

Conversation

jakkdl commented Jul 5, 2023

codecov bot commented Jul 5, 2023 • edited Loading

Codecov Report

A5rocks commented Jul 6, 2023

jakkdl Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

jakkdl Jul 6, 2023

Choose a reason for hiding this comment

jakkdl commented Jul 6, 2023

jakkdl commented Jul 6, 2023

A5rocks left a comment

Choose a reason for hiding this comment

A5rocks Jul 7, 2023

Choose a reason for hiding this comment

jakkdl Jul 7, 2023

Choose a reason for hiding this comment

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

Kludex commented Jul 7, 2023

jakkdl commented Jul 7, 2023

codecov bot commented Jul 5, 2023 •

edited

Loading

jakkdl Jul 6, 2023 •

edited

Loading