Enable pyupgrade rule #2809

CoolCat467 · 2023-10-03T05:11:57Z

This PR enables the pyupgrade ruff rule and fixes all the existing issues that were raised.

codecov · 2023-10-03T05:16:14Z

Codecov Report

Merging #2809 (641fa56) into master (9ec9d05) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2809   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files         115      115           
  Lines       17482    17482           
  Branches     3113     3113           
=======================================
  Hits        17336    17336           
  Misses        101      101           
  Partials       45       45

Files	Coverage Δ
trio/_channel.py	`100.00% <100.00%> (ø)`
trio/_core/_io_epoll.py	`100.00% <100.00%> (ø)`
trio/_core/_io_windows.py	`98.80% <100.00%> (ø)`
trio/_core/_run.py	`100.00% <100.00%> (ø)`
trio/_core/_windows_cffi.py	`100.00% <100.00%> (ø)`
trio/_file_io.py	`100.00% <ø> (ø)`
trio/_path.py	`100.00% <ø> (ø)`
trio/_subprocess.py	`98.29% <ø> (ø)`
trio/_subprocess_platform/kqueue.py	`100.00% <100.00%> (ø)`
trio/_sync.py	`100.00% <100.00%> (ø)`
... and 4 more

A5rocks

Couple of comments but overall looks good

A5rocks · 2023-10-04T04:05:22Z

trio/_core/_io_epoll.py

@@ -201,7 +201,7 @@ class _EpollStatistics:
 class EpollIOManager:
    _epoll: select.epoll = attr.ib(factory=select.epoll)
    # {fd: EpollWaiters}
-    _registered: DefaultDict[int, EpollWaiters] = attr.ib(
+    _registered: defaultdict[int, EpollWaiters] = attr.ib(


Isn't defaultdict a 3.9+ thing? Why is ruff doing this? Cause the from __future__ import annotations? or is this a bug.

It’s probably that yes. There’s an option to decide whether to preserve the hints making sense at runtime.

Yes, it decides what's allowed based on both the version you specify and if a future annotations import exists.

There is a setting to make sure runtime typing works, mostly so a specific library can work properly, but in my opinion I agree with ruff's behavior here. Why have outdated versions of the type annotations if we don't have to worry about runtime type annotations?

It's been waffled about a lot in various PR's, without a central decision actually getting taken: #2687
IMHO if we want runtime typing we should have tests for it, and enforce it with linter rules.

A5rocks · 2023-10-04T04:06:01Z

trio/_core/_io_windows.py

-    current_op: Optional[AFDPollOp] = attr.ib(default=None)
+    read_task: _core.Task | None = attr.ib(default=None)
+    write_task: _core.Task | None = attr.ib(default=None)
+    current_op: AFDPollOp | None = attr.ib(default=None)


Eh, same as above. I won't make any more comments about 3.9+ features sneaking in.

If we are going to require runtime typing, on 3.8, and that disallowing using | in type hints, then I vehemently disagree with doing that. This is currently the status quo in a lot of the code base.

A5rocks · 2023-10-04T04:07:24Z

trio/_dtls.py

-                SSL.OP_NO_QUERY_MTU
-                | SSL.OP_NO_RENEGOTIATION  # type: ignore[attr-defined]
-            )
+            SSL.OP_NO_QUERY_MTU | SSL.OP_NO_RENEGOTIATION  # type: ignore[attr-defined]


Huh, why did ruff change this? I think it was intentionally formatted to showcase which specific attribute wasn't defined.

I saw this and the diff and was a bit confused myself, not sure about this one.

Another one of my PRs hit this and I looked into it further, this change was by black, not ruff.

trio/_tests/type_tests/check_wraps.py

A5rocks · 2023-10-04T04:09:27Z

trio/_util.py

-                "  trio.run({async_fn.__name__}(...))            # incorrect!\n"
-                "  nursery.start_soon({async_fn.__name__}(...))  # incorrect!\n"
+                f"  trio.run({async_fn.__name__}(...))            # incorrect!\n"
+                f"  nursery.start_soon({async_fn.__name__}(...))  # incorrect!\n"


Mind checking the commit that these came in with (or the PR more generally) and seeing if there's other stuff like this?

Same string concat weirdness (see other comment)

A5rocks · 2023-10-04T04:10:28Z

trio/_util.py

                "library written for asyncio/twisted/tornado or similar? "
-                "That won't work without some sort of compatibility shim.".format(coro)
+                "That won't work without some sort of compatibility shim."


Wow messed up upgrade bugs everywhere. Mind checking the commit/PR that this came in with too?

I don't think it's actually a bug, I think the format call would have worked for it previously because it was applying to the concatenation of all of the strings, since they have parenthesis wrapping them. It definitely looks kind of strange though, and I think using f-strings here makes it much more clear what the idea is than this format call that looks a bit out of place.

jakkdl

imo we can merge this, but Somebody ™️ needs to take a final decision on runtime typing. That affects much more code than this though, so I don't think it should hold up this one.

jakkdl · 2023-10-19T13:08:59Z

trio/_core/_io_epoll.py

@@ -201,7 +201,7 @@ class _EpollStatistics:
 class EpollIOManager:
    _epoll: select.epoll = attr.ib(factory=select.epoll)
    # {fd: EpollWaiters}
-    _registered: DefaultDict[int, EpollWaiters] = attr.ib(
+    _registered: defaultdict[int, EpollWaiters] = attr.ib(


It's been waffled about a lot in various PR's, without a central decision actually getting taken: #2687
IMHO if we want runtime typing we should have tests for it, and enforce it with linter rules.

jakkdl · 2023-10-19T13:10:58Z

trio/_core/_io_windows.py

-    current_op: Optional[AFDPollOp] = attr.ib(default=None)
+    read_task: _core.Task | None = attr.ib(default=None)
+    write_task: _core.Task | None = attr.ib(default=None)
+    current_op: AFDPollOp | None = attr.ib(default=None)


If we are going to require runtime typing, on 3.8, and that disallowing using | in type hints, then I vehemently disagree with doing that. This is currently the status quo in a lot of the code base.

jakkdl · 2023-10-23T09:38:09Z

You can merge this after fixing B904.
The codecov fail looks dumb, windows pypy-3.9-nightly is still bad, and macOS 3.10 is hitting the timeout issues that have started plaguing us. So none of those are related to the PR.

CoolCat467 · 2023-10-24T00:01:39Z

pre-commit.ci autofix

This reverts commit e275513.

Enable pyupgrade rule

4af786b

CoolCat467 added 2 commits October 3, 2023 00:29

Fix imports being seperated

c6a32f2

No quotes required, we have future annotations imported

fab4a08

A5rocks reviewed Oct 4, 2023

View reviewed changes

CoolCat467 added 3 commits October 4, 2023 23:36

Merge branch 'master' into enable-pyupgrade

c31e0a6

Merge branch 'master' into enable-pyupgrade

bccd2ed

Fix UP036 Version block is outdated for minimum Python version

5cbcb46

CoolCat467 mentioned this pull request Oct 19, 2023

Type all remaining tests #2820

Merged

CoolCat467 requested a review from A5rocks October 19, 2023 00:46

jakkdl approved these changes Oct 19, 2023

View reviewed changes

Merge branch 'master' into enable-pyupgrade

6891b40

Fix B904 error

2ce22e7

CoolCat467 enabled auto-merge (squash) October 23, 2023 23:28

Merge branch 'master' into enable-pyupgrade

3e74044

CoolCat467 disabled auto-merge October 24, 2023 00:01

pre-commit-ci bot and others added 3 commits October 24, 2023 00:01

[pre-commit.ci] auto fixes from pre-commit.com hooks

e275513

Revert "[pre-commit.ci] auto fixes from pre-commit.com hooks"

32c4037

This reverts commit e275513.

Fix accidental revert from merge

dc1153e

CoolCat467 enabled auto-merge (squash) October 24, 2023 00:04

Merge branch 'master' into enable-pyupgrade

641fa56

CoolCat467 merged commit f85021e into python-trio:master Oct 25, 2023
31 of 32 checks passed

CoolCat467 deleted the enable-pyupgrade branch October 26, 2023 03:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable pyupgrade rule #2809

Enable pyupgrade rule #2809

CoolCat467 commented Oct 3, 2023

codecov bot commented Oct 3, 2023 •

edited

Loading

A5rocks left a comment

A5rocks Oct 4, 2023

TeamSpen210 Oct 4, 2023

CoolCat467 Oct 5, 2023

CoolCat467 Oct 5, 2023

jakkdl Oct 19, 2023

A5rocks Oct 4, 2023

jakkdl Oct 19, 2023

A5rocks Oct 4, 2023

CoolCat467 Oct 5, 2023

CoolCat467 Oct 19, 2023

A5rocks Oct 4, 2023

CoolCat467 Oct 5, 2023

A5rocks Oct 4, 2023

CoolCat467 Oct 5, 2023

jakkdl left a comment

jakkdl Oct 19, 2023

jakkdl Oct 19, 2023

jakkdl commented Oct 23, 2023

CoolCat467 commented Oct 24, 2023

Enable pyupgrade rule #2809

Enable pyupgrade rule #2809

Conversation

CoolCat467 commented Oct 3, 2023

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

A5rocks left a 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

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

jakkdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Oct 23, 2023

CoolCat467 commented Oct 24, 2023

codecov bot commented Oct 3, 2023 •

edited

Loading