-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pylint] Implement auto-fix for missing-maxsplit-arg (PLC0207)
#19387
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
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLC0207 | 124 | 62 | 62 | 0 | 0 |
## Summary This came up on [Discord](https://discord.com/channels/1039017663004942429/1343692072921731082/1395447082520678440) and also in #19387, but on macOS the tmp directory is a symlink to `/private/tmp`, which breaks this filter. I'm still not quite sure why only these tests are affected when we use the `tempdir_filter` elsewhere, but hopefully this fixes the immediate issue. Just `tempdir.path().canonicalize()` also worked, but I used `dunce` since that's what I saw in other tests (I guess it's not _just_ these tests). Some related links from uv: - https://github.com/astral-sh/uv/blob/1b2f212e8b2f91069b858cb7f5905589c9d15add/crates/uv/tests/it/common/mod.rs#L1161-L1178 - https://github.com/astral-sh/uv/blob/1b2f212e8b2f91069b858cb7f5905589c9d15add/crates/uv/tests/it/common/mod.rs#L424-L438 - astral-sh/uv#14290 Thanks to @zanieb for those! ## Test Plan I tested the `main` branch on my MacBook and reproduced the test failure, then confirmed that the tests pass after the change. Now to make sure it passes on Windows, which caused most of the trouble in the first PR!
0ecc0ed to
d5d89c2
Compare
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking good! I just had some ideas/questions about the diagnostic messages and fix safety.
| if actual_split_type == suggested_split_type { | ||
| format!("Pass `maxsplit=1` into `str.{actual_split_type}()`") | ||
| } else { | ||
| format!("Use `str.{suggested_split_type}()` and pass `maxsplit=1`") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look quite repetitive with the message above, especially the first case, which is exactly the same. How hard would it be to make a message like:
Replace with `{suggested_split_type}(..., maxsplit=1)`.
I don't really love that either, but I don't think we should duplicate the message. Ideally we could include the actual variable name and pattern, but that would require a bit of extra work.
Another option might be changing message to something more general instead. I think the more detailed suggestion is a better fit here in fix_title, especially since it's AlwaysFixable.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the message and fix_title are redundant and that it's better to have more detail in the fix_title. I also agree that including the actual variable name and pattern is ideal, but I think that might fall out of the scope for this PR. Perhaps we go with a more general message and leave that as a potential follow-up?
I think since this is AlwaysFixable we can get away with a general message especially because the fix_title will always be there to provide more detail. How does that sound to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me!
| SliceBoundary::Last => "rsplit", | ||
| }; | ||
|
|
||
| let maxsplit_argument_edit = fix::edits::add_argument( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking if other calls to add_argument were safe or not, and I found a few examples like this:
ruff/crates/ruff_linter/src/rules/pylint/rules/subprocess_run_without_check.rs
Lines 77 to 93 in cabdd96
| add_argument( | |
| "check=False", | |
| &call.arguments, | |
| checker.comment_ranges(), | |
| checker.locator().contents(), | |
| ), | |
| // If the function call contains `**kwargs`, mark the fix as unsafe. | |
| if call | |
| .arguments | |
| .keywords | |
| .iter() | |
| .any(|keyword| keyword.arg.is_none()) | |
| { | |
| Applicability::Unsafe | |
| } else { | |
| Applicability::Safe | |
| }, |
I think we should probably do the same here and also add a ## Fix safety section to the docs. I can't think of another reason it would be unsafe because we shouldn't be able to delete any comments with these edits.
Here's a PR adding one of these, for a ## Fix safety example too:
https://github.com/astral-sh/ruff/pull/9176/files
...les/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0207_missing_maxsplit_arg.py.snap
Show resolved
Hide resolved
...les/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0207_missing_maxsplit_arg.py.snap
Show resolved
Hide resolved
|
Thanks for the comments, in addition to the updated |
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Summary
As a follow-up to #18949 (suggested here), this PR implements auto-fix logic for
PLC0207.Test Plan
Existing tests pass, with updates to the snapshot so that it expects the new output that comes along with the auto-fix.