Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore ruff/tryceratops rule TRY003 #4454

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Conversation

DimitriPapadopoulos
Copy link
Contributor

Summary of changes

TRY003 Avoid specifying long messages outside the exception class

Applying this rule would mean creating lots of specialised exception classes. Not sure this would improve readability and maintainability in this context.

Addresses #4450 (comment).

Pull Request Checklist

TRY003 Avoid specifying long messages outside the exception class

Applying this rule would mean creating lots of specialised exception
classes. Not sure this would improve readability and maintainability
in this context.
@Avasam
Copy link
Contributor

Avasam commented Jul 9, 2024

First case of a PR failing because of this: #4464

Probably related to https://github.com/businho/pytest-ruff/releases/tag/v0.4.0 being released ~1h ago.

CC @jaraco @abravalheri to try and expedite this quick fix

@jaraco
Copy link
Member

jaraco commented Jul 9, 2024

I did confirm that downgrading to pytest-ruff 0.3.2 suppresses the errors. I don't see anything in the pytest-ruff changelog that indicates that new rules are now enforced. Was it a mistake? Shouldn't we first see if pytest-ruff wants to yank the version? Is this issue going to affect other projects?

@jaraco
Copy link
Member

jaraco commented Jul 9, 2024

I see now there are probably several factors at play. Something about #4450 probably introduced broken config (or maybe there's broken config elsewhere) such that the failures weren't introduced, but now that pytest-ruff 0.4.0 fixes the handling of the brokenness in the config, the ruff checks are now running and failing. Maybe?

In any case, it looks like the issue is likely isolated to setuptools and its custom config so won't affect other projects right away.

Let's bring this fix in.

Since it seems we've already encountered scope creep on the TRY jobs, maybe it makes sense to opt-in to desired rules rather than opt out?

@jaraco jaraco merged commit 04b3bb9 into pypa:main Jul 9, 2024
22 checks passed
@DimitriPapadopoulos
Copy link
Contributor Author

I noticed TRY003 appearing with ruff 0.5. I suspect the underlying version of ruff has been bumped to 0.5. I don't know how the version of ruff is chosen by pytest-ruff.

@DimitriPapadopoulos DimitriPapadopoulos deleted the TRY003 branch July 9, 2024 20:58
@Avasam
Copy link
Contributor

Avasam commented Jul 9, 2024

Independent of pytest-ruff, TRY003 is a code that is raised locally, as it should given current configs (that's how I noticed it, and should've been excluded in #4450).
Why it didn't raise on the CI previously is still an open question.

I noticed TRY003 appearing with ruff 0.5. I suspect the underlying version of ruff has been bumped to 0.5. I don't know how the version of ruff is chosen by pytest-ruff.

This here PR used pytest-ruff==0.3.2 and ruff==0.5.0 when last run: https://github.com/pypa/setuptools/actions/runs/9756635721/job/26927344059?pr=4454#step:5:26

@DimitriPapadopoulos
Copy link
Contributor Author

My hope is that ruff will stabilise and such issues will get less and less frequent. The downside of opt-in is that the opt-in list might be much larger than the opt-out list, and would need to be updated from time to time to include new rules. Perhaps wait and see how often new rules break havoc?

@Avasam
Copy link
Contributor

Avasam commented Jul 9, 2024

Btw @jaraco TRY003 isn't new at all, it's been there since https://github.com/astral-sh/ruff/releases/tag/v0.0.236 , and I don't see any mention of it since ruff 0.1 https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md

My best guess is that pytest-ruff 0.4 did fix something that was hiding errors


@DimitriPapadopoulos 's #4450 might've perfectly arrived as we started using Ruff 0.5 https://github.com/pypa/setuptools/actions/runs/9724519803/job/26840815339#step:5:26

Ruff 0.5 wasn't compatible with pytest-ruff 0.3.2 apparently businho/pytest-ruff#23 (not sure why in our case it silently passed instead of failing though). But that would explain why it passed the CI until some fixes in pytest-ruff 0.4

No idea how @DimitriPapadopoulos didn't get TRY003 locally, because I get it on setuptool's repo as far back as ruff 0.1.8 (maybe you ran through tox / pytest-ruff ? )

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jul 9, 2024

After downgrading to ruff 0.3, I still see TRY003 errors:

$ ruff --version
ruff 0.3.2
$ 
$ ruff check --extend-select TRY
[...]
Found 72 errors.
$ 
$ ruff --version
ruff 0.5.1
$ 
$ ruff check --extend-select TRY
[...]
Found 72 errors.
$ 

Why I didn't experience TRY003 errors in the CI jobs after #4450 remains a mystery... I didn't experience TRY003 errors locally either, not until you told me about them and I upgraded ruff.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jul 9, 2024

(maybe you ran through tox / pytest-ruff ? )

Yes, I seem to recall I had initially run pytest-ruff, not ruff. That's why!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants