python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
6.22k stars 344 forks source link

Disable ruff's COM812 #3144

Open A5rocks opened 3 hours ago

A5rocks commented 3 hours ago

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 commented 2 hours ago

I thought that https://github.com/python-trio/trio/pull/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 commented 2 hours ago

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:

CoolCat467 commented 2 hours ago

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 commented 2 hours ago

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[bot] commented 2 hours ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.62%. Comparing base (532ba5f) to head (95679fb). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3144 +/- ## ======================================= Coverage 99.62% 99.62% ======================================= Files 122 122 Lines 18400 18402 +2 Branches 1223 1223 ======================================= + Hits 18331 18333 +2 Misses 47 47 Partials 22 22 ``` [see 1 file with indirect coverage changes](https://app.codecov.io/gh/python-trio/trio/pull/3144/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio)