[pylint] Narrow diagnostic range and exclude cases without exception handlers (PLW0717)#25440
Merged
Conversation
…n handlers (`PLW0717`) Summary -- This PR fixes #25438 by narrowing the diagnostic range from the entire `try` statement to only the `try` keyword. This was one of the two alternatives mentioned on #25438, and I thought this looked a bit nicer than only narrowing the range to the `try` body. I think this is also consistent with the range for `too-many-statements` (`PLR0915`), which marks the function name: https://play.ruff.rs/dafe23a1-2e89-48a5-b4d7-b0825b737152 I'm happy to reconsider, though. Another possible alternative along these lines is marking the first statement that exceeds the limit, or the last statement, or something like that. This PR also fixes #25390 by limiting the rule to `try` statements with `except` handlers. Either an `except` or a `finally` clause is required, so this avoids emitting diagnostics for context-manager-like cases, such as the one reported in the issue: ```py session = ... try: print() print() print() print() print() print() finally: session.close() ``` where the `try` is just ensuring that some cleanup is performed and can't actually trigger the bad behavior described in the rule docs of mistakenly catching the wrong exception. Test Plan -- New mdtests based on the issues
I was initially grabbing the token, but codex pointed out this pattern in a couple other rules: - https://github.com/astral-sh/ruff/blob/1243ce1748e5e903f1b0f1963102d281bc798dc3/crates/ruff_linter/src/rules/flake8_simplify/rules/suppressible_exception.rs#L175 - https://github.com/astral-sh/ruff/blob/1243ce1748e5e903f1b0f1963102d281bc798dc3/crates/ruff_linter/src/rules/flake8_bandit/rules/assert_used.rs#L48
ntBre
added
rule
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLW0717 | 380 | 0 | 380 | 0 | 0 |
Contributor
Author
|
Wow this pattern is way more common than I noticed on the original PR. This looks like a good change. |
MichaReiser
approved these changes
May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes #25438 by narrowing the diagnostic range from the entire
trystatement to only thetrykeyword. This was one of the two alternatives mentioned on #25438, and I thought this looked abit nicer than only narrowing the range to the
trybody. I think this is also consistent with therange for
too-many-statements(PLR0915), which marks the function name:https://play.ruff.rs/dafe23a1-2e89-48a5-b4d7-b0825b737152
I'm happy to reconsider, though. Another possible alternative along these lines is marking the first
statement that exceeds the limit, or the last statement, or something like that.
This PR also fixes #25390 by limiting the rule to
trystatements withexcepthandlers. Either anexceptor afinallyclause is required, so this avoids emitting diagnostics forcontext-manager-like cases, such as the one reported in the issue:
where the
tryis just ensuring that some cleanup is performed and can't actually trigger the badbehavior described in the rule docs of mistakenly catching the wrong exception.
Test Plan
New mdtests based on the issues