[pylint] Implement too-many-try-statements (W0717)#23970
Conversation
That sounds perfect! This is looking good from a very quick skim. Let me know if you need any help! I'll approve the CI runs in case it's helpful. |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLW0717 | 1921 | 1921 | 0 | 0 | 0 |
|
Not being familiar with the original rule, both the name and the diagnostic message seem very confusing/misleading to me. "Too many try statements" feels like it's complaining about how many separate try/except blocks there are, not how many statements are within a single try block. I'd suggest something more like "Too many statements in try block" or similar. Looks like pylint uses "Try clause contains too many statements". |
|
I agree, this name is terrible. Is there a way I could rename the rule but keep an alias to the original name for discoverability or should I just clarify the documentation for it ? |
|
Since this is a new-to-Ruff rule, simply rename the rule struct. I don't think we are tied to the existing rule names from other linters (though we do preserve the existing rule code). |
da05928 to
304c358
Compare
|
Update:
I'll get started on tests promptly. |
|
So I was planning to mimick # t.py
def random_ratio() -> float:
try:
a = randint(-100, 100)
b = randint(-100, 100)
c = randint(-100, 100)
d = randint(-100, 100)
scale = randint(1, 5)
res = scale * (a + b) / (c + d)
except ZeroDivisionError:
return random_ratio()
else:
return resThe only problem I see here is that the generated error points to the wrong place, but I don't know how to fix that, please advise. |
ntBre
left a comment
There was a problem hiding this comment.
Thank you! I only had a couple of small suggestions, but this looks great to me overall. I think moving the tests along with the helper function makes sense.
Let's make sure we look through the ecosystem results (I'll approve the workflow again) too. I expect this to be a pretty noisy/pedantic rule with a lot of hits, but it should also be accurate since we're just reusing num_statements. Just good to double check.
|
thanks @ntBre ! |
I noticed that too, but I think I'm happy with the more conservative default. It might be easiest to note this in the example for the configuration option. Something like: # use the default from pylint
max-try-statements = 1 |
|
Gotcha. Will push this in a minute |
|
@ntBre any update ? |
|
rebased to refresh CI |
ntBre
left a comment
There was a problem hiding this comment.
Thanks again and sorry for the delay. I had a couple more really small nits, and we need to add some tests. I didn't notice that before. It looks like other pylint rules are here:
ruff/crates/ruff_linter/src/rules/pylint/mod.rs
Lines 240 to 242 in e89a8dc
|
Tests added, and took your suggestion into account. Thank you ! |
c13704d to
bc60b9f
Compare
|
I just rebased the branch again, but the only change was the inclusion of test files I forgot to include last time. Before this is considered for merge though, I still would like to ask whether it's okay to rename the public parameter for clarity ( |
Thanks! Don't worry about the I do think |
|
Nice. I'll push again in a couple minutes then |
|
(I think I overestimated my travel laptop's capacity to run tests in ordinate time, but the patch is still coming) |
|
I was also wondering if I should rename any files, or the rule itself, as was discussed early on, but I can't tell what's the actual source of truth in this aspect. |
|
The source of truth for the rule name is the I'm also happy to push any snapshot updates/test fixes if needed :) I'll kick off CI again. |
|
I like this name even more indeed. Do feel free to push to my branch if anything trivial is still missing or incorrect ! |
ntBre
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks again for all of your work here and for your patience with reviews!
…3970) ## Summary Close astral-sh#23810 Opening as a draft mostly to signal that I'm getting this going. At the time of opening, and as far as I can tell, this *compiles* correctly locally, but it's missing tests and docs. I also have a question I would like to ask @ntBre at this early stage of the PR: it seemed to me that the logic from `too_many_statements::num_statements` could be entirely re-used, and I didn't want to duplicate it, so I moved it up the module tree to `ruff_linter::rules::pylint::helpers`. Is this the prefered approach, or is there a better way I should implement this code re-use ? ## Test Plan
…3970) ## Summary Close astral-sh#23810 Opening as a draft mostly to signal that I'm getting this going. At the time of opening, and as far as I can tell, this *compiles* correctly locally, but it's missing tests and docs. I also have a question I would like to ask @ntBre at this early stage of the PR: it seemed to me that the logic from `too_many_statements::num_statements` could be entirely re-used, and I didn't want to duplicate it, so I moved it up the module tree to `ruff_linter::rules::pylint::helpers`. Is this the prefered approach, or is there a better way I should implement this code re-use ? ## Test Plan
Summary
Close #23810
Opening as a draft mostly to signal that I'm getting this going. At the time of opening, and as far as I can tell, this compiles correctly locally, but it's missing tests and docs.
I also have a question I would like to ask @ntBre at this early stage of the PR: it seemed to me that the logic from
too_many_statements::num_statementscould be entirely re-used, and I didn't want to duplicate it, so I moved it up the module tree toruff_linter::rules::pylint::helpers. Is this the prefered approach, or is there a better way I should implement this code re-use ?Test Plan