-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pylint] Update missing-maxsplit-arg docs and error to suggest proper usage (PLC0207)
#18949
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 | 92 | 46 | 46 | 0 | 0 |
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 don't think this really addresses the problem in the issue. The confusion isn't from the rule applying to either split or rsplit calls, it's the fact that the fix might change which call you need.
This is the confusing case from the issue:
url = "www.example.com"
url.split(".")[-1] # result is "com"And the diagnostic from Ruff:
-:2:1: PLC0207 Accessing only the first or last element of `str.split()` without setting `maxsplit=1`
|
1 | url = "www.example.com"
2 | url.split(".")[-1] # result is "com"
| ^^^^^^^^^^^^^^^^^^ PLC0207
|
The error message implies that the fix is simply to add maxsplit=1, but that gives a different result:
```python
url = "www.example.com"
url.split(".", maxsplit=1)[-1] # result is now "example.com" instead of "com"The correct fix is to add maxplit=1 and change split to rsplit, restoring the original behavior.
That's the kind of information we need to include in the rule documentation and in the error message. In the latter case, we'll need to inspect the slice ([0] or [-1]) and call type (split or rsplit) and customize the error message accordingly so that we actually suggest the right thing for the specific situation.
|
Thanks for the comments, that makes sense! I just patched some updates to address them! |
| let correct_function = match index { | ||
| 0 => "split", | ||
| -1 => "rsplit", | ||
| _ => "", // We should never hit this case |
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.
This should probably be an unreachable! instead, since if that case is hit it should cause a failure instead of a silent issue.
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 think we should probably define a simple enum instead of using an i64. That way we can check for known values up front and bail out of the whole diagnostic. Then we can match exhaustively here.
...les/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0207_missing_maxsplit_arg.py.snap
Show resolved
Hide resolved
missing-maxsplit-arg docs and error to explain rsplit usagemissing-maxsplit-arg docs and error to suggest proper split/rsplit usage along with maxsplit=1
missing-maxsplit-arg docs and error to suggest proper split/rsplit usage along with maxsplit=1missing-maxsplit-arg docs and error to suggest proper usage
|
Hey @ntBre — just wanted to check in on this. I’ve pushed some updates addressing your feedback above. When you get the chance, let me know if there’s anything else you’d like me to adjust. Thanks again! 🙂 |
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.
This looks great, thank you! And thanks for the ping.
I'm pretty sure these changes give us all the information we need to generate an autofix too. Feel free to work on that in a follow-up PR if you're interested :)
missing-maxsplit-arg docs and error to suggest proper usagepylint] Update missing-maxsplit-arg docs and error to suggest proper usage (PLC0207)
…re_help * 'main' of https://github.com/astral-sh/ruff: (34 commits) [docs] add capital one to who's using ruff (astral-sh#19248) [`pyupgrade`] Keyword arguments in `super` should suppress the `UP008` fix (astral-sh#19131) [`flake8-use-pathlib`] Add autofixes for `PTH100`, `PTH106`, `PTH107`, `PTH108`, `PTH110`, `PTH111`, `PTH112`, `PTH113`, `PTH114`, `PTH115`, `PTH117`, `PTH119`, `PTH120` (astral-sh#19213) [ty] Do not run `mypy_primer.yaml` when all changed files are Markdown files (astral-sh#19244) [`flake8-bandit`] Make example error out-of-the-box (`S412`) (astral-sh#19241) [`pydoclint`] Make example error out-of-the-box (`DOC501`) (astral-sh#19218) [ty] Add "kind" to completion suggestions [ty] Add type information to `all_members` API [ty] Expand API of `all_members` to return a struct [ty] Ecosystem analyzer PR comment workflow (astral-sh#19237) [ty] Merge `ty_macros` into `ruff_macros` (astral-sh#19229) [ty] Fix `ClassLiteral.into_callable` for dataclasses (astral-sh#19192) [ty] `dataclasses.field` support (astral-sh#19140) [ty] Fix panic for attribute expressions with empty value (astral-sh#19069) [ty] Return `CallableType` from `BoundMethodType.into_callable_type` (astral-sh#19193) [`flake8-bugbear`] Support non-context-manager calls in `B017` (astral-sh#19063) [ty] Improved diagnostic for reassignments of `Final` symbols (astral-sh#19214) [ty] Use full range for assignment definitions (astral-sh#19211) [`pylint`] Update `missing-maxsplit-arg` docs and error to suggest proper usage (`PLC0207`) (astral-sh#18949) [ty] Add `set -eu` to mypy-primer script (astral-sh#19212) ... # Conflicts: # crates/ty_python_semantic/src/types/class.rs
…19387) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary As a follow-up to #18949 (suggested [here](#18949 (review))), this PR implements auto-fix logic for `PLC0207`. ## Test Plan <!-- How was it tested? --> Existing tests pass, with updates to the snapshot so that it expects the new output that comes along with the auto-fix.
Summary
Fix #18383 by updating the documentation and error message to explain that users should use
rsplitin order to access the last element of the result withmaxsplit=1Test Plan
Only documentation and an error message was changed. As such, snapshots were updated to reflect the new error message. With this change, all existing tests pass.