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

Enforce ruff/tryceratops rules (TRY) #266

Merged
merged 4 commits into from
Aug 26, 2024
Merged

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Jun 29, 2024

I have not applied this one:

TRY003 Avoid specifying long messages outside the exception class

I believe it changes too much code, so I have left it out. I am happy to apply the TRY003 rule as well if you want me too.

Comment on lines +148 to +149
else:
return (dst, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I feel like TRY is overreaching here. There's no way that return (dst, 1) is going to cause an OSError, and re-writing with the else clause just distracts from the typical control flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this rule might help identify cases where it's not clear which part of the code may throw. The usefulness of the TRY300 rule depends on the context: number of lines and complexity of the code under try.

In this specific case, it is quite clear that the return line won't raise en exception. On the other hand, I do not find the else clause less readable. The typical control flow is important of course, but so is clarity about what is tested under try. Additionally, the error control flow feels important too, enough to have 3 lines of comments.

TRY300 Consider moving this statement to an `else` block
TRY301 Abstract `raise` to an inner function
@jaraco jaraco merged commit d42a6aa into pypa:main Aug 26, 2024
19 checks passed
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.

2 participants