Skip to content
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

C415 doc and/or implementation is broken #7773

Closed
JelleZijlstra opened this issue Oct 3, 2023 · 4 comments · Fixed by #7774
Closed

C415 doc and/or implementation is broken #7773

JelleZijlstra opened this issue Oct 3, 2023 · 4 comments · Fixed by #7774
Labels
documentation Improvements or additions to documentation

Comments

@JelleZijlstra
Copy link
Contributor

The documentation for C415 (https://docs.astral.sh/ruff/rules/unnecessary-subscript-reversal/) states that it makes the following changes:

Examples:
reversed(iterable[::-1])
set(iterable[::-1])
sorted(iterable)[::-1]

Use instead:
reversed(iterable)
set(iterable)
sorted(iterable, reverse=True)

But unless I'm doing something wrong, it doesn't:

$ cat rev.py 
def f(iterable):
    x = reversed(iterable[::-1])
    y = set(iterable[::-1])
    z = sorted(iterable)[::-1]
    return x, y, z
$ python -m ruff check --select=C415 --fix --diff rev.py 
$ python -m ruff --version
ruff 0.0.292

Notice no warnings.

The reason I was checking this in the first place was that the first example (changing reversed(iterable[::-1]) to reversed(iterable)) is obviously wrong: if Ruff did that, it would result in the eventual order of the output being the wrong way. Possibly Ruff should output just iterable in this case, though it doesn't feel like a common issue.

@charliermarsh
Copy link
Member

Yeah, that's definitely wrong.

@charliermarsh charliermarsh added the documentation Improvements or additions to documentation label Oct 3, 2023
@charliermarsh
Copy link
Member

Need to figure out whether it's the docs, implementation, or both. Thanks!

@JelleZijlstra
Copy link
Contributor Author

And I'm indeed doing something wrong: if --fix is passed, Ruff doesn't print warnings.

$ python -m ruff check rev.py --select=C4
rev.py:2:9: C415 Unnecessary subscript reversal of iterable within `reversed()`
rev.py:3:9: C415 Unnecessary subscript reversal of iterable within `set()`

But that still leaves two issues:

  • The doc example about reversed() is wrong
  • The implementation doesn't complain about the sorted() pattern the doc mentions

@charliermarsh
Copy link
Member

(Ruff does print warnings with --fix; I think it's --diff that suppresses the warnings, since the purpose of that flag is to show fix changes.)

charliermarsh added a commit that referenced this issue Oct 3, 2023
## Summary

Two of the three listed examples were wrong: one was semantically
incorrect, another was _correct_ but not actually within the scope of
the rule.

Good motivation for us to start linting documentation examples :)

Closes #7773.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants