chore: drop Python 2 syntax #526
Conversation
|
These are great, but please don't do this on the Awkward codebase yet: I'm in the middle of a "change everything" PR (scikit-hep/awkward#1184) that wouldn't merge well with this sort of thing. |
|
Okay, no problem. Will stick with this. :) I'm going to do a couple more changes (I should be able to detect and replace I think the main question: do you want to chain or hide the ModuleNotFoundErrors in extras? |
|
I've also got a few more changes I want to annotate above, so I put it into draft |
henryiii
force-pushed
the
henryiii/chore/py3
branch
from
e1706a9 to
cb84e7c
Compare
henryiii
force-pushed
the
henryiii/chore/py3
branch
from
a13035e to
4f928a7
Compare
| uproot._util.py36, | ||
| sys.version_info < (3, 7), |
There was a problem hiding this comment.
I dropped this one helper; the sys.version_info is much easier to handle statically (pyupgrade can remove branches when it uses this syntax, for example), reads better (you don't have to look up the meaning of py36), and was the only place this helper was used.
There was a problem hiding this comment.
There was a pack of py26, py27, py35, py36 booleans, as well as an iswin. The py36 was only needed in one case, which is why I still consider Python 3.6 acceptable—it needs very few work-arounds.
Meanwhile, there are a lot of different ways of saying sys.version_info[0] == 3 and sys.version_info[1] < 7, etc. (which mean subtly different things). This tuple-inequality idiom is succinct, and if it's recognized by static checkers, then it's the one to use. But I didn't know that there was a "one to use."
Similarly for the Windows-check: iswin has been redefined a few times as some ways of checking platform haven't been as robust as the one we have there now. By putting it in a boolean, we could fix it centrally.
Another one is isstr, which was a lot more important when we had to deal with different meanings of str, bytes, and unicode.
The isint is one that I think should stay. There isn't a good idiom for "integer but not boolean." There was some version of NumPy where NumPy scalars weren't being recognized as integers (can't remember which, exactly) and anyway Python and NumPy differ on whether booleans should be considered integers. Having a central facility for that makes it possible to control from one location.
| return lzma | ||
| else: | ||
| return lzma | ||
| return lzma |
There was a problem hiding this comment.
This function is kind of pointless now, don't know if you want to deprecate it.
There was a problem hiding this comment.
I had that in the back of my mind: we don't have to get lzma from extras.py anymore; it can be a top-of-the-module import. I think it only appears in compression.py.
|
Okay, I think I'm done with this. Some things (like + -> strings) could be worked on in the future, but don't need to be here. |
|
I scanned through all the changes and it looks good! With so many changes, the real proof that it's safe is in the tests. After it's merged, the Coffea tests should pick it up (is that right, @nsmith-?) and as a squashed commit, we have the option to revert it if there's a problem. Thank you very much! (Also for that note about when |
|
Yes coffea requires |
First commit is mostly automated, second commend is hand cleanup for things the first one didn't catch. Many
.format's are left, this is just the easy ones that clearly are simpler as f-strings. Manual comparisons with sys.version_info are always preferred, and can be automatically cleaned up, so I had to look around for the try/catch ones. I'll note some cleanups inline.