Enable ruff flake8-simplify rule #2823

CoolCat467 · 2023-10-19T02:09:23Z

This PR enables the ruff flake8-simplify rule (SIM) and fixes all of the new errors from enabling it.

codecov · 2023-10-19T02:12:15Z

Codecov Report

Merging #2823 (69cc316) into master (8b15548) will increase coverage by 0.02%.
The diff coverage is 95.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2823      +/-   ##
==========================================
+ Coverage   99.16%   99.18%   +0.02%     
==========================================
  Files         115      115              
  Lines       17514    17466      -48     
  Branches     3125     3140      +15     
==========================================
- Hits        17368    17324      -44     
+ Misses        101       97       -4     
  Partials       45       45

Files	Coverage Δ
trio/_core/_io_epoll.py	`100.00% <100.00%> (ø)`
trio/_core/_multierror.py	`100.00% <ø> (ø)`
trio/_core/_run.py	`100.00% <100.00%> (ø)`
trio/_core/_tests/test_asyncgen.py	`99.08% <100.00%> (-0.01%)`	⬇️
trio/_core/_tests/test_guest_mode.py	`99.69% <100.00%> (-0.01%)`	⬇️
trio/_core/_tests/test_multierror.py	`100.00% <100.00%> (ø)`
trio/_core/_tests/test_run.py	`100.00% <100.00%> (ø)`
trio/_core/_wakeup_socketpair.py	`100.00% <100.00%> (ø)`
trio/_dtls.py	`96.76% <100.00%> (-0.02%)`	⬇️
trio/_highlevel_open_tcp_stream.py	`97.59% <100.00%> (-0.06%)`	⬇️
... and 14 more

A5rocks

I think overall this set of lints is kinda silly. Horizontal space really really shouldn't matter, especially with an autoformatter.

I can kinda see how a few might be helpful (e.g. suppress... and maybe multiple with statements) but talking about the multiple with statements thing, the stance here is super inconsistent.

Finally, I really don't get the rule that reorders the == or >= things.

trio/_core/_multierror.py

trio/_core/_run.py

A5rocks · 2023-10-19T03:23:31Z

trio/_core/_tests/test_asyncgen.py

@@ -44,14 +43,14 @@ async def async_main() -> None:
        with pytest.warns(
            ResourceWarning, match="Async generator.*collected before.*exhausted"
        ):
-            assert 42 == await example("abandoned").asend(None)
+            assert await example("abandoned").asend(None) == 42


I don't get how this simplifies anything?

I like this as a general style thing, though I think the developers of this repo already adheres to it decently well. E.g. code like

if (a == 3 || 4 == b): ...

is very silly, and in spirit feels like something Black would enforce - except they are explicitly AST-neutral. So I think there's good that there's a tool that enforces it.

Oh, I actually very much prefer the changes to this set of comparisons. Because they go against the standard, I was thinking maybe it's because the developer wants to place an emphasis on the left term because the right term is constant ... but no the literal is just a random magic number and the expression on the right hand side very much does change.

trio/_core/_tests/test_multierror.py

trio/_core/_tests/test_run.py

A5rocks · 2023-10-19T03:30:53Z

trio/_subprocess.py

@@ -171,7 +172,11 @@ def __init__(
            else:
                # It worked! Wrap the raw fd up in a Python file object to
                # make sure it'll get closed.
-                self._pidfd = open(fd)
+                # fmt: off
+                self._pidfd = open(  # noqa: SIM115  # open-file-with-context-handler


I think ruff shouldn't error on this case, this is just silly. Not every class should be constructable solely using a context manager.

Yeah this one could be reported upstream. Why is the # fmt: off needed ?

# fmt: off needed because basically ruff and black end up fighting over the formatting if I remember correctly. I think it's because of differences in the way the two handle trying to make it not exceed a line number limit.

that also sounds like something that should be reported upstream 😅

if we're fmt: offing then I think fd) should be moved onto line 176, kinda silly to split it up this much.

A5rocks · 2023-10-19T03:32:39Z

trio/_tests/test_highlevel_open_tcp_stream.py

@@ -250,7 +245,7 @@ def __init__(
        assert len(set(ip_order)) == len(ip_list)
        ip_dict: dict[str | int, tuple[float, str]] = {}
        for ip, delay, result in ip_list:
-            assert 0 <= delay
+            assert delay >= 0


I don't get this.

I like the change

trio/_tests/test_path.py

jakkdl

I think the combine-with and combine-if checks should be disabled and the changes reverted:

They suppress coverage info
when the ifs/withs are doing completely different types of things, squashing them together makes them seem related - which they aren't. This is especially bad with pytest.raises + testing a with statement, and just in general with a library like Trio where careful thinking about scopes is really important.

jakkdl · 2023-10-23T09:17:02Z

trio/_core/_tests/test_asyncgen.py

@@ -44,14 +43,14 @@ async def async_main() -> None:
        with pytest.warns(
            ResourceWarning, match="Async generator.*collected before.*exhausted"
        ):
-            assert 42 == await example("abandoned").asend(None)
+            assert await example("abandoned").asend(None) == 42


I like this as a general style thing, though I think the developers of this repo already adheres to it decently well. E.g. code like

if (a == 3 || 4 == b): ...

is very silly, and in spirit feels like something Black would enforce - except they are explicitly AST-neutral. So I think there's good that there's a tool that enforces it.

jakkdl · 2023-10-23T09:18:59Z

trio/_core/_tests/test_asyncgen.py

@@ -44,14 +43,14 @@ async def async_main() -> None:
        with pytest.warns(
            ResourceWarning, match="Async generator.*collected before.*exhausted"
        ):
-            assert 42 == await example("abandoned").asend(None)
+            assert await example("abandoned").asend(None) == 42


Oh, I actually very much prefer the changes to this set of comparisons. Because they go against the standard, I was thinking maybe it's because the developer wants to place an emphasis on the left term because the right term is constant ... but no the literal is just a random magic number and the expression on the right hand side very much does change.

jakkdl · 2023-10-23T09:19:21Z

trio/_core/_tests/test_guest_mode.py

@@ -593,10 +594,8 @@ async def agen(label):
            yield 1
        finally:
            library = sniffio.current_async_library()
-            try:
+            with contextlib.suppress(trio.Cancelled):


TIL. Me like

jakkdl · 2023-10-23T09:21:28Z

trio/_subprocess.py

@@ -171,7 +172,11 @@ def __init__(
            else:
                # It worked! Wrap the raw fd up in a Python file object to
                # make sure it'll get closed.
-                self._pidfd = open(fd)
+                # fmt: off
+                self._pidfd = open(  # noqa: SIM115  # open-file-with-context-handler


Yeah this one could be reported upstream. Why is the # fmt: off needed ?

jakkdl · 2023-10-23T09:23:19Z

trio/_tests/test_dtls.py

@@ -173,7 +170,7 @@ async def route_packet(packet: UDPPacket) -> None:
                        break

            def route_packet_wrapper(packet: UDPPacket) -> None:
-                try:
+                try:  # noqa: SIM105  # suppressible-exception


loss of coverage information on whether a suppressed except is caught or not is actually a pretty good reason not to enable this check.

trio/_tests/test_dtls.py

jakkdl · 2023-10-23T09:24:44Z

trio/_tests/test_highlevel_open_tcp_stream.py

@@ -250,7 +245,7 @@ def __init__(
        assert len(set(ip_order)) == len(ip_list)
        ip_dict: dict[str | int, tuple[float, str]] = {}
        for ip, delay, result in ip_list:
-            assert 0 <= delay
+            assert delay >= 0


I like the change

trio/_core/_multierror.py

CoolCat467 · 2023-10-23T22:31:59Z

I think the combine-with and combine-if checks should be disabled and the changes reverted:

1. They suppress coverage info

2. when the ifs/withs are doing completely different types of things, squashing them together makes them seem related - which they aren't. This is especially bad with `pytest.raises` + testing a with statement, and just in general with a library like Trio where careful thinking about scopes is really important.

Reverted as of 76d7413

trio/_core/_multierror.py

…cket"

Collapsable if check is a good thing, but care should be used to check that it doesn't make something confusing, which is why it's a `--unsafe-fixes` option

jakkdl

Minor formatting change, and I think you should open upstream issues. Otherwise looks great~

jakkdl · 2023-10-25T11:33:52Z

trio/_subprocess.py

@@ -171,7 +172,11 @@ def __init__(
            else:
                # It worked! Wrap the raw fd up in a Python file object to
                # make sure it'll get closed.
-                self._pidfd = open(fd)
+                # fmt: off
+                self._pidfd = open(  # noqa: SIM115  # open-file-with-context-handler


that also sounds like something that should be reported upstream 😅

if we're fmt: offing then I think fd) should be moved onto line 176, kinda silly to split it up this much.

…verride block

jakkdl · 2023-10-26T13:42:30Z

I think we can merge this unless @A5rocks or others have any comments.

TeamSpen210

Looks like improvements, though we might want to use a bit of caution with contextlib.suppress. It's definitely tidier, but I imagine it could have a performance impact.

Enable ruff flake8-simplify rule

94d11a2

A5rocks reviewed Oct 19, 2023

View reviewed changes

jakkdl requested changes Oct 23, 2023

View reviewed changes

CoolCat467 added 2 commits October 23, 2023 16:24

Merge branch 'master' into enable-flake8-simplify

e55b1e3

Revert collapsable-if and multiple-with-statements changes

76d7413

CoolCat467 requested a review from jakkdl October 23, 2023 22:32

CoolCat467 added 2 commits October 23, 2023 17:38

Fix "pyright_verifytypes can't see contexlib in trio.socket"

5e3f3cc

Update outdated argument name

7fffe21

A5rocks reviewed Oct 23, 2023

View reviewed changes

trio/_core/_multierror.py Outdated Show resolved Hide resolved

CoolCat467 added 6 commits October 23, 2023 19:11

Merge branch 'master' into enable-flake8-simplify

e94a1cb

Merge branch 'master' into enable-flake8-simplify

3220d6d

2nd try at fixing "pyright_verifytypes can't see contexlib in trio.so…

9ba8e61

…cket"

Fix mypy error

dd7b0ed

Remove unused import

69fbe9b

Revert disabling collapsable if check

37e7638

Collapsable if check is a good thing, but care should be used to check that it doesn't make something confusing, which is why it's a `--unsafe-fixes` option

CoolCat467 requested a review from A5rocks October 25, 2023 05:21

jakkdl approved these changes Oct 25, 2023

View reviewed changes

CoolCat467 and others added 3 commits October 25, 2023 23:10

Move explain comment to line above, allowing us to remove no format o…

4c66af5

…verride block

Merge branch 'master' into enable-flake8-simplify

1ec117b

Merge branch 'master' into enable-flake8-simplify

a50ae1f

TeamSpen210 approved these changes Oct 27, 2023

View reviewed changes

jakkdl and others added 2 commits October 27, 2023 14:27

Merge branch 'master' into enable-flake8-simplify

fe3495b

Merge branch 'master' into enable-flake8-simplify

69cc316

jakkdl merged commit 2a28313 into python-trio:master Oct 30, 2023
28 of 30 checks passed

CoolCat467 deleted the enable-flake8-simplify branch October 30, 2023 21:12

jakkdl mentioned this pull request Aug 5, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ruff flake8-simplify rule #2823

Enable ruff flake8-simplify rule #2823

CoolCat467 commented Oct 19, 2023

codecov bot commented Oct 19, 2023 •

edited

Loading

A5rocks left a comment

A5rocks Oct 19, 2023

jakkdl Oct 23, 2023

jakkdl Oct 23, 2023

A5rocks Oct 19, 2023

jakkdl Oct 23, 2023

CoolCat467 Oct 23, 2023

jakkdl Oct 25, 2023

A5rocks Oct 19, 2023

jakkdl Oct 23, 2023

jakkdl left a comment

jakkdl Oct 23, 2023

jakkdl Oct 23, 2023

jakkdl Oct 23, 2023

jakkdl Oct 23, 2023

jakkdl Oct 23, 2023

jakkdl Oct 23, 2023

CoolCat467 commented Oct 23, 2023

jakkdl left a comment

jakkdl Oct 25, 2023

jakkdl commented Oct 26, 2023

TeamSpen210 left a comment

Enable ruff flake8-simplify rule #2823

Enable ruff flake8-simplify rule #2823

Conversation

CoolCat467 commented Oct 19, 2023

codecov bot commented Oct 19, 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

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

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

CoolCat467 commented Oct 23, 2023

jakkdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Oct 26, 2023

TeamSpen210 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2023 •

edited

Loading