Switch to use ruff #2795

CoolCat467 · 2023-09-08T01:40:12Z

This PR has the project switch to using ruff instead of flake8 and handles the slight differences it has.

They have a quite compelling advertisement of sorts of why you should use their system in their readme, but in the end I use it myself in a lot of my projects because it's got nearly all the warnings flake8 does but also has autofix support, and it's way faster.

codecov · 2023-09-08T01:49:49Z

Codecov Report

Merging #2795 (c53f3d1) into master (c30c76f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2795   +/-   ##
=======================================
  Coverage   99.13%   99.13%           
=======================================
  Files         115      115           
  Lines       17206    17229   +23     
  Branches     3082     3084    +2     
=======================================
+ Hits        17057    17080   +23     
  Misses        104      104           
  Partials       45       45

Files	Coverage Δ
trio/__init__.py	`100.00% <ø> (ø)`
trio/_channel.py	`100.00% <100.00%> (ø)`
trio/_core/_instrumentation.py	`100.00% <100.00%> (ø)`
trio/_core/_multierror.py	`100.00% <100.00%> (ø)`
trio/_core/_run.py	`100.00% <100.00%> (ø)`
trio/_core/_tests/test_asyncgen.py	`99.09% <100.00%> (ø)`
trio/_core/_tests/test_guest_mode.py	`99.69% <ø> (ø)`
trio/_core/_tests/test_io.py	`100.00% <100.00%> (ø)`
trio/_core/_tests/test_ki.py	`97.82% <100.00%> (ø)`
trio/_core/_tests/test_run.py	`100.00% <100.00%> (ø)`
... and 16 more

A5rocks · 2023-09-08T12:21:22Z

Opinion: ruff not supporting custom plugins is kind of a big deal for a project like trio, where we try to test, lint, etc. to fix future problems.

But I haven't used ruff myself so I may be wrong to focus on that.

jakkdl · 2023-09-08T21:31:14Z

ruff is surprisingly on top of mirroring the - functionality in a lot of flake8 plugins, I added a check to flake8-bugbear and in less than 24 hours it was added to ruff. But if there's a flake8 plugin we need that's not implemented I don't mind having flake8 just for that one plugin

CoolCat467 · 2023-09-10T01:36:48Z

ruff is surprisingly on top of mirroring the - functionality in a lot of flake8 plugins, I added a check to flake8-bugbear and in less than 24 hours it was added to ruff. But if there's a flake8 plugin we need that's not implemented I don't mind having flake8 just for that one plugin

Is there anything I need to change in this PR for this? I didn't know trio used any plugins

A5rocks

Questions about configuration are probably because I don't know much about ruff.

.pre-commit-config.yaml

check.sh

A5rocks · 2023-09-12T17:09:23Z

trio/_subprocess_platform/__init__.py

-        # noqa'd as it's an exported symbol
+        # as it's an exported symbol, noqa'd


Is this something we should report to ruff? (something like "hey look if the comment doesn't start with noqa: and it isn't just noqa and the line is blank, it's probably just an explanation"?)

astral-sh/ruff#7333

trio/__init__.py

pyproject.toml

A5rocks · 2023-09-12T17:15:46Z

Is there anything I need to change in this PR for this? I didn't know trio used any plugins

I was more thinking about the open issue to make a flake8 plugin (which eventually spun out to make flake8-trio but ... maybe we want to include something in trio or something), in which case we'd probably want to run it against trio too.

But as jakkdl pointed out, we can just add that in at the time of.

A5rocks · 2023-09-12T21:12:19Z

Quick note that ruff now doubles up as an autoformatter; let's hold off on using that until it's not in alpha :^)

jakkdl · 2023-09-13T12:25:54Z

Is there anything I need to change in this PR for this? I didn't know trio used any plugins

I was more thinking about the open issue to make a flake8 plugin (which eventually spun out to make flake8-trio but ... maybe we want to include something in trio or something), in which case we'd probably want to run it against trio too.

But as jakkdl pointed out, we can just add that in at the time of.

fun fact: flake8-trio can now be run standalone: https://github.com/Zac-HD/flake8-trio#install-and-run-as-standalone
This is because doing autofixes while running as a flake8 plugin wasn't possible.

pyproject.toml

trio/__init__.py

jakkdl · 2023-09-13T13:02:04Z

trio/_subprocess_platform/__init__.py

-        # noqa'd as it's an exported symbol
+        # as it's an exported symbol, noqa'd


astral-sh/ruff#7333

.pre-commit-config.yaml

check.sh

jakkdl · 2023-09-13T13:08:38Z

ci.sh

-    # TODO: remove this once we have a py.typed file
-    touch "$INSTALLDIR/py.typed"
-


If being a stickler I'd remove all the instances of generating/removing py.typed in a dedicated PR.

pyproject.toml

charliermarsh · 2023-09-27T03:12:00Z

You can allow certain ambiguous characters via allowed-confusables:

[tool.ruff]
allowed-confusables = ["–"]

This reverts commit a756800.

A5rocks

Accidentally reverted the noqa explanation fix

trio/_tests/test_highlevel_ssl_helpers.py

jakkdl

I'd recommend reverting the – changes - I think that's the more correct form of dash, using the ASCII character is just a poor substitute.

While I don't disagree in theory, I think standardizing and limiting the character set in code (as opposed to e.g. documentation) is very important for the sake of diffs and sanity of developers not used to differentiating them/don't have an easy way of inputting em-dash/using some bad font that makes them look similar/identical. If it was only allowed in docstrings and comments I'd be less hesitant, though either way is probably fine.

trio/_tools/gen_exports.py

setup.py

Co-authored-by: jakkdl <[email protected]>

A5rocks

Really rough (haha, ruff) review. After these are resolved this should look good to me though.

A5rocks · 2023-09-30T04:29:27Z

test-requirements.in

@@ -13,8 +13,7 @@ cryptography>=41.0.0  # cryptography<41 segfaults on pypy3.10
 black; implementation_name == "cpython"
 mypy; implementation_name == "cpython"
 types-pyOpenSSL; implementation_name == "cpython" # and annotations
-flake8
-flake8-pyproject
+ruff >= 0.0.291


Unless we're depending on some 0.0.291 functionality, I think we should not have a version constraint here -- the lockfile can have that for us!

--format is depreciated, 0.0.291 has the new version, --output-format dcce938

trio/_core/_multierror.py

A5rocks · 2023-09-30T04:30:59Z

trio/_core/_run.py

+
+
 @attr.s(eq=False, hash=False, slots=True)


Hmm unintentional new whitespace?

This is a part of one of the formatting rules that says that there should always be two newlines between any new definitions in the global scope

A5rocks · 2023-09-30T04:34:00Z

trio/_tests/tools/test_gen_exports.py

+        == '''def combine_and(data: list[str]) -> str:
+    """Join values of text, and have 'and' with the last one properly."""
+    if len(data) >= 2:
+        data[-1] = 'and ' + data[-1]
+    if len(data) > 2:
+        return ', '.join(data)
+    return ' '.join(data)'''


To my untrained eye this looks the same as the file above. Maybe maybe a variable so you only have to type it once?

Also, just wondering where this sample code comes from!

Checked and it is indeed identical, will make a variable shortly.

Sample code was in one of the files I have on my desktop when I'm testing things, but digging deeper it looks like a modified version of one of the functions from my discord bot (https://github.com/CoolCat467/StatusBot/blob/4c4a0808ecbba4bcd3a1a6f0bff8169bc8a2bbd5/src/StatusBot/bot.py#L169C1-L176C26)

Edit: To be more precise, this function's idea is used all over the place in a lot of my projects, usually when displaying lists of something to end users in a nice way, and is usually either specialized in joining the end with and or with or, and in a few iterations it's generic and can have the end be one of the inputs. combine_and, combine_end, and list_or are all examples of different names I've used for this idea.

Made variable in af0547a

trio/_tools/gen_exports.py

A5rocks · 2023-10-03T01:55:50Z

This looks good!

CoolCat467 added 3 commits September 7, 2023 20:35

Switch to ruff and deal with the slight differences

828ff80

Project must have a name

25dddba

Remove project specifier, still using setup.py

228194d

CoolCat467 added 4 commits September 7, 2023 20:50

We have a py.typed file now

a7df9b1

Update test requirements for python 3.8

25fe2fb

Add ruff and remove flake8 and isort

dfc8820

Fix only posting fixes, check for all errors

78d67c1

CoolCat467 marked this pull request as ready for review September 8, 2023 02:11

CoolCat467 changed the title ~~[WIP] Switch to use ruff~~ Switch to use ruff Sep 8, 2023

CoolCat467 requested review from jakkdl, A5rocks and TeamSpen210 September 8, 2023 02:14

CoolCat467 added the dependencies Pull requests that update a dependency file label Sep 8, 2023

A5rocks requested changes Sep 12, 2023

View reviewed changes

jakkdl requested changes Sep 13, 2023

View reviewed changes

CoolCat467 and others added 8 commits September 20, 2023 00:44

Merge branch 'master' into replace-with-ruff

0a4486b

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

2a4aff1

Disable autofix and skip nothing

1239a4b

Fixes pointed out from reviews

8193e23

Enable Error rules

1a67d10

Stop ignoring undefined-name errors

06bcb00

Fix _value_factory change side effects

1efd80e

Change lint failure test with isort removed now

59a0a8c

Attempt to fix again

7e5122a

CoolCat467 added 5 commits September 26, 2023 22:20

Print stderr for debugging

defb75b

Revert "Enable RUFF rule and apply fixes"

2d60883

This reverts commit a756800.

Re-enable RUFF rule but without confusables change

1c1dbf7

Check response code differently

bef1141

Messed up format argument, it's different in check

961eb13

A5rocks reviewed Sep 27, 2023

View reviewed changes

trio/_tests/test_highlevel_ssl_helpers.py Outdated Show resolved Hide resolved

trio/_tests/test_highlevel_ssl_helpers.py Outdated Show resolved Hide resolved

Fix comments that mentioned noqa (ruff regex bug) II

6cc59b7

CoolCat467 requested a review from A5rocks September 27, 2023 04:11

Merge branch 'master' into replace-with-ruff

211dd8d

jakkdl approved these changes Sep 28, 2023

View reviewed changes

trio/_tools/gen_exports.py Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

CoolCat467 and others added 7 commits September 28, 2023 19:27

Apply suggestions from code review

d364693

Co-authored-by: jakkdl <[email protected]>

Fix explanation comments (more accurate information)

8b8dc70

Merge branch 'master' into replace-with-ruff

1812bf7

Merge branch 'master' into replace-with-ruff

7ed8e69

Update ruff version requirement, --format is deprecated

dcce938

Fix merge conflicts in gen_exports

951d42a

Update test-requirements.txt

54ee74d

A5rocks approved these changes Sep 30, 2023

View reviewed changes

CoolCat467 added 5 commits September 30, 2023 18:49

Deduplicate sample test code segment

af0547a

Update docstrings

18e7668

Put docstring ends on newlines

60fbe57

Merge branch 'master' into replace-with-ruff

ddd6bc5

Update test-requirements.txt

90c5de7

charliermarsh mentioned this pull request Oct 1, 2023

Avoid undefined-name errors for recursive calls after deletions astral-sh/ruff#7733

Open

Add reference to ruff issue to explanation comments

c53f3d1

A5rocks merged commit bbb4be7 into python-trio:master Oct 3, 2023
27 of 28 checks passed

CoolCat467 deleted the replace-with-ruff branch October 3, 2023 03:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to use ruff #2795

Switch to use ruff #2795

CoolCat467 commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

A5rocks commented Sep 8, 2023

jakkdl commented Sep 8, 2023

CoolCat467 commented Sep 10, 2023

A5rocks left a comment

A5rocks Sep 12, 2023

jakkdl Sep 13, 2023

A5rocks commented Sep 12, 2023

A5rocks commented Sep 12, 2023

jakkdl commented Sep 13, 2023

jakkdl Sep 13, 2023

jakkdl Sep 13, 2023

charliermarsh commented Sep 27, 2023

A5rocks left a comment

jakkdl left a comment

A5rocks left a comment

A5rocks Sep 30, 2023

CoolCat467 Sep 30, 2023

A5rocks Sep 30, 2023

CoolCat467 Sep 30, 2023

A5rocks Sep 30, 2023

CoolCat467 Sep 30, 2023 •

edited

Loading

CoolCat467 Oct 1, 2023

A5rocks commented Oct 3, 2023

		# noqa'd as it's an exported symbol
		# as it's an exported symbol, noqa'd

		# TODO: remove this once we have a py.typed file
		touch "$INSTALLDIR/py.typed"

Switch to use ruff #2795

Switch to use ruff #2795

Conversation

CoolCat467 commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

A5rocks commented Sep 8, 2023

jakkdl commented Sep 8, 2023

CoolCat467 commented Sep 10, 2023

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

A5rocks commented Sep 12, 2023

A5rocks commented Sep 12, 2023

jakkdl commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Sep 27, 2023

A5rocks left a comment

Choose a reason for hiding this comment

jakkdl left a comment

Choose a reason for hiding this comment

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

CoolCat467 Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A5rocks commented Oct 3, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

CoolCat467 Sep 30, 2023 •

edited

Loading