Disable ruff's COM812

[A5rocks]

Ruff's documentation recommends this be disabled with their formatter. I assume that advice holds for black too. The other rule they recommend disabling is ISC001 which I don't think black has trouble with.

I don't see a better way to keep this up to date, but the way I noticed this probably works. I was making changes and pre-commit was making me run git commit thrice, as the ruff autofixes didn't pass the next round of black. The sequence was git commit: black -> ruff (because ordering), then git commit: black, then git commit: it actually worked.

[CoolCat467]

I thought that #3044 was specifically for enabling this check, cannot say I support disabling it. I have noticed that some changes require multiple pre-commit runs as well, and while slightly annoying at times I don't think it merits disabling this rule entirely.

[A5rocks]

I thought that #3044 was specifically for enabling this check, cannot say I support disabling it. I have noticed that some changes require multiple pre-commit runs as well, and while slightly annoying at times I don't think it merits disabling this rule entirely.

I wasn't sure if there's other lints in that. But the alternatives are:

• move the black run after ruff in the pre-commit file. this is alright, but also implies we're heavily relying on black's magic comma rule which I really dislike.
• this

[CoolCat467]

Now, if the rules created an edit loop that didn't resolve after a few iterations, then I might be for this change, but as is I think the commas apply in a way that makes the code a bit more readable, especially in cases with functions with several arguments.

[A5rocks]

I'm not sure the alternative (reorder checks) works. We could duplicate the black check. This is because it seems ruff is whitespace sensitive. So take for instance this:

nested_array = [
    [
        1,
        2,
        3,
    ]
]

Under black's new style, black -> ruff will yield (supposedly, I can't get this preview style working...):

nested_array = [[
    1,
    2,
    3,
]]

Whereas ruff -> black will yield:

nested_array = [
    [
        1,
        2,
        3,
    ],
]

---

I'll change this to duplicating the black entry in pre-commit. (to be clear: I think the best solution is just to disable COM812 but I also haven't really noticed it much since it's always just in pre-commit, so I'm not sure how much it helps/hurts).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (b43383d) to head (88bbadc).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3144   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files         124      124           
  Lines       18381    18381           
  Branches     1226     1226           
=======================================
  Hits        18312    18312           
  Misses         47       47           
  Partials       22       22           

[jakkdl]

I personally prefer disabling COM812 over duplicating black. COM812 is perhaps nice, and ofc no reason to undo the changes in the PR, but I don't think it's worth having to duplicate black.

Under black's new style, black -> ruff will yield (supposedly, I can't get this preview style working...):

from the link:

You can use a magic trailing comma to avoid this compacting behavior; by default, Black will not reformat the following code:

So I think because you have a trailing comma on the 3 you're not getting any changes from it.

If we can't make both ruff & black happy simultaneously I strongly think COM812 should be disabled, as we're otherwise gonna introduce headaches for LSP and/or IDE users that might be raising errors and/or autoformatting code.

[A5rocks]

OK, so first... relitigating COM812. I ran black -C src/trio (which ignores all trailing commas), see the diff:

A5rocks/trio@disable-trailing-comma...86b17b2

Then I ran ruff check src/trio --unsafe-fixes and black src/trio (and then ruff check src/trio and black src/trio again because... well this is a good case against COM812, since it popped up after the first cycle) and got this diff (this is, in theory, the result of COM812 and probably little else):

A5rocks/trio@86b17b2...6ac28c9

Interestingly, there's a decently large diff combining the two commits above. I won't make a PR for this because formatting really doesn't matter that much:

A5rocks/trio@disable-trailing-comma...A5rocks:trio:commaless

For reference, here's the code snippet that required two cycles:

RaisesGroup = ...
Matcher = ...


def f() -> None:
    with RaisesGroup(
        Matcher(ValueError, "error text", lambda e: isinstance(e.__context__, KeyError))
    ):
        ...

(.venv) PS C:\Users\A5rocks\Documents\trio> ruff check test.py
Found 1 error (1 fixed, 0 remaining).
(.venv) PS C:\Users\A5rocks\Documents\trio> black test.py
reformatted test.py

All done! ✨ 🍰 ✨
1 file reformatted.
(.venv) PS C:\Users\A5rocks\Documents\trio> ruff check test.py
Found 1 error (1 fixed, 0 remaining).
(.venv) PS C:\Users\A5rocks\Documents\trio> black test.py
reformatted test.py

All done! ✨ 🍰 ✨
1 file reformatted.
(.venv) PS C:\Users\A5rocks\Documents\trio> ruff check test.py
All checks passed!

Note: I'm pretty sure this is a line length thing so I don't know if a 3 cycle input is possible, and this kind of input is likely rare.

---

I was expecting to be disappointed with the changes COM812 gives, but the changes are... fine. However, very few of the changes from COM812 (second link above) feel like they'd affect a diff: how much are we really modifying function signatures? I don't think changes other than that are useful.

I still think it should be disabled but I'm less sure of myself now... In isolation I think it works well and if it didn't conflict with black then I'm fine with it (more than I was when I approved the previous PR!). But duplicating black in the pre-commit file is a hack and yeah, having pre-commit avoid this (which it wouldn't based on my above experience of needing ruff -> black -> ruff -> black before stable output) doesn't help people relying on autoformatting in the command line or in their editor.

---

So I think because you have a trailing comma on the 3 you're not getting any changes from it.

Not sure how I tested this before but it actually does work with the trailing comma on 3 now that I try it again... so, oh well.

[jakkdl]

yeah let's go with disabling it then. No shame in trying something out and taking it back after deliberation, better than being overly afraid of doing stuff because we don't wanna change our minds :)

You can also remove the extra black run from gen_exports.py introduced in #3044
https://github.com/CoolCat467/trio/blob/30babafa732799d6730fe5936c5ddbe8f453ad12/src/trio/_tools/gen_exports.py#L194-L197

[CoolCat467]

Yea, I understand disabling this now. That is very unfortunate it doesn't play well with black.

[webknjaz]

Does Ruff have human-readable rule names? I thought it did... If so, I'd rather use those over some alphanumeric contextless IDs.

[A5rocks]

I don't think it does, astral-sh/ruff#1773 is still open (but I haven't read through it)

[jakkdl]

ye they still can't be used ~anywhere

[jakkdl]

bumping/clarifying my previous comment:

You can also remove the extra black run from gen_exports.py introduced in #3044
https://github.com/CoolCat467/trio/blob/30babafa732799d6730fe5936c5ddbe8f453ad12/src/trio/_tools/gen_exports.py#L194-L197

gen_exports.py currently runs black->ruff->black, which shouldn't be necessary without COM812