[flake8-bugbear] point previous occurrence (B033)#22634
[flake8-bugbear] point previous occurrence (B033)#22634ntBre merged 5 commits intoastral-sh:mainfrom
flake8-bugbear] point previous occurrence (B033)#22634Conversation
|
crates/ruff_linter/src/rules/flake8_bugbear/rules/duplicate_value.rs
Outdated
Show resolved
Hide resolved
|
Is there an example showing what would happen if there were three occurrences of the same value in a single set, eg |
…lue.rs Co-authored-by: Amethyst Reese <amethyst@n7.gg>
It always consider the immediately previous element as the previous occurrence. $ echo '{1, 1, 1}' | cargo run -p ruff -- check --select B033 -
B033 [*] Sets should not contain duplicate item `1`
--> -:1:2
|
1 | {1, 1, 1}
| - ^ duplicated here
| |
| previous occurrence here
|
help: Remove duplicate item
B033 [*] Sets should not contain duplicate item `1`
--> -:1:5
|
1 | {1, 1, 1}
| - ^ duplicated here
| |
| previous occurrence here
|
help: Remove duplicate item
Found 2 errors.
[*] 2 fixable with the `--fix` option. |
ruff] point previous occurrence in B033flake8-bugbear] point previous occurrence in (B033)
flake8-bugbear] point previous occurrence in (B033)flake8-bugbear] point previous occurrence (B033)
| value.range(), | ||
| ); | ||
| diagnostic.secondary_annotation(format_args!("previous occurrence here"), existing); | ||
| diagnostic.set_primary_message(format_args!("duplicated here")); |
There was a problem hiding this comment.
I think this is in line with the two other uses of set_primary_message in Ruff, but one thing to consider here is that the primary annotation message is appended to the concise diagnostic message:
❯ just run check --select B033 --output-format concise - <<<"s = {1, 2, 1}"
-:1:12: B033 [*] Sets should not contain duplicate item `1`: duplicated hereI think this looks slightly awkward, so I would lean towards omitting the primary message, even though it looks nicer in the full output. But I'm curious what you and @amyreese think!
There was a problem hiding this comment.
Perhaps we could move the item X out of the diagnostic message in some way, so that it reads more like:
Set contains duplicates: item `X`
There was a problem hiding this comment.
@amyreese I didn’t fully understand. Is this your intended message?
echo "s = {1, 2, 1}" | cargo run -p ruff -- check --select B033 --output-format concise -
-:1:12: B033 [*] Set contains duplicates: item `X`For consistency, here is the current output for a duplicate in __all__:
echo "__all__ = ['a', 'a']" | cargo run -p ruff -- check --preview --select RUF068 --output-format concise -
-:1:17: RUF068 [*] `__all__` contains duplicate entries: `a` duplicated here
There was a problem hiding this comment.
I tested and it seems that removing the primary message works best here. I added a commit removing the primary message.
Full Message
> echo "s = {True, 2, 1}" | cargo run -p ruff -- check --select B033 -
B033 [*] Sets should not contain duplicate items, but `True` and `1` has the same value
--> -:1:6
|
1 | s = {True, 2, 1}
| ---- ^
| |
| previous occurrence here
|
help: Remove duplicate item
Found 1 error.
[*] 1 fixable with the `--fix` option.Concise message
> echo "s = {True, 2, 1}" | cargo run -p ruff -- check --select B033 --output-format concise -
-:1:15: B033 [*] Sets should not contain duplicate items, but `True` and `1` has the same value
Found 1 error.
[*] 1 fixable with the `--fix` option.There was a problem hiding this comment.
@ntBre if you liked this version more, I can remove the primary message from RUF068 as well.
The primary message was leaving the concise message inconsistent
ntBre
left a comment
There was a problem hiding this comment.
Let's move ahead with the current state of the PR. I think it's okay to prioritize the full diagnostic over the concise or other output formats, and I like the way the full diagnostic looks. I pushed one commit capitalizing the P in the secondary annotation, but this is otherwise good to go. Thank you!
Improves B033 linting error by pointing to the previous occurrence of
the duplicate item.
# Before
```text
B033 [*] Sets should not contain duplicate item `"value1"`
--> B033.py:4:35
4 | incorrect_set = {"value1", 23, 5, "value1"}
| ^^^^^^^^
help: Remove duplicate item
- incorrect_set = {"value1", 23, 5, "value1"}
4 + incorrect_set = {"value1", 23, 5}
```
# After
```text
B033 [*] Sets should not contain duplicate item `"value1"`
--> B033.py:4:18
4 | incorrect_set = {"value1", 23, 5, "value1"}
| -------- ^^^^^^^^ duplicated here
| |
| previous occurrence here
help: Remove duplicate item
- incorrect_set = {"value1", 23, 5, "value1"}
4 + incorrect_set = {"value1", 23, 5}
```
---------
Co-authored-by: Amethyst Reese <amethyst@n7.gg>
Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
Improves B033 linting error by pointing to the previous occurrence of the duplicate item.
Before
After