[pyflakes] Add fix safety section (F841)
#17410
Conversation
|
A small question: Do we need to list all possible cases that may lead to modifications in the comments, and ensure that there are test cases for all of them? |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks for working on this!
This rule is a little tricky since it's always unsafe and there's not much commentary in the file. Did you try finding the original PR adding the rule? That could be a good place to find more discussion. I tried a bit and didn't find it, though.
If we can't find any further discussion, I think what you have is fine, despite my comment on the side effects point below.
A small question: Do we need to list all possible cases that may lead to modifications in the comments, and ensure that there are test cases for all of them?
I think just a couple of reasons for the unsafety is good, like you have here. I think I'd also lean away from modifying code or tests in these PRs and just add docs for what's there now. On the other hand, if you add tests that help you determine when the fix is safe or not, we might as well include them! (That may be more applicable for rules that are sometimes safe, unlike this one)
The only potential insecurity of this rule seems to be that it deletes comments (if any) when removing content. I've reviewed several other rules, and they all appear to behave similarly. For such cases, do we just need to list the unique associations related to the comments? |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thank you! One more tiny stylistic nit. I think the list format might be good for rules with many possible unsafe conditions, but we typically keep these as regular paragraphs.
I think what you have here is great. For some other rules, you could list a couple of examples of places where comments could be deleted if you want, but most existing rules I found just say that comments could be deleted without more explanation, so that's fine to do too. |
|
@ntBre Thank you very much for your help! It has been incredibly helpful in deepening my understanding of these matters. Initially, I tried to identify all possible points of impact, but apart from the deletion of comments, I didn’t find any others—the same goes for the other files. Moving forward, I will strive to conduct more thorough checks and carefully review the other rules. I believe this is a good start. Once again, I truly appreciate your efforts! |
|
No problem, thanks for working on this! I'll give your other PRs a look soon. I pushed one small commit just updating the test snapshot that shows the rule documentation. Then I'll merge right after the checks finish! |
|
ntBre
changed the title
pyflakes] Add fix safety section (F841)
Thank you for your help! I'm not sure if I checked everything thoroughly, but most of them don't have side effects. |
* main: [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406)
* main: (123 commits) [red-knot] Handle explicit class specialization in type expressions (#17434) [red-knot] allow assignment expression in call compare narrowing (#17461) [red-knot] fix building unions with literals and AlwaysTruthy/AlwaysFalsy (#17451) [red-knot] Type narrowing for assertions (take 2) (#17345) [red-knot] class bases are not affected by __future__.annotations (#17456) [red-knot] Add support for overloaded functions (#17366) [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444) [red-knot] more type-narrowing in match statements (#17302) [red-knot] Add some narrowing for assignment expressions (#17448) [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446) Server: Use `min` instead of `max` to limit the number of threads (#17421) [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406) [red-knot] Super-basic generic inference at call sites (#17301) ...
3 participants
add fix safety section to docs for #15584, I'm new to ruff and not sure if the content of this PR is correct, but I hope it can be helpful.