-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[RUF005] Unexpectedly removes comments #2054
Comments
Gonna avoid autofixing for now in cases that contain comments. |
Thanks for the quick fix! |
I also have a question on this @charliermarsh. Thought I should make another issue but first wanna try here. What's the reason behind this warning? The only advantage I know of unpacking is it can work for something like adding list to tuple, but in that case it should be used when needed explicitly. |
I think the way I'd like to answer that question is by clarifying the position Ruff that is taking by including rules (or not). I generally try to stay somewhat objective with regards to what gets included in Ruff, looking at the following factors:
To me, this rule satisfied criteria 1-3 (I can understand why folks would want to enforce this). 4 is harder to measure in this case, vs. when reimplementing flake8 plugins, where I can see how widely used they are. By including a rule in Ruff, I don't mean to express the opinion that you should have it enabled (there are rules in Ruff that I wouldn't enforce in my own projects, like the One thing I have been thinking, about and that this rule arguably raises (as with any new rule), is that if you enable a category like |
Hmm I don't dislike seeing new stuff like this @charliermarsh enabled. If it's not auto-enabled I would probably miss it. Seeing it like this, I get a choice. Let it run (it's auto-fixable so easy to do), or put it in my Let me maybe explain what I did when I saw this applied to my project to maybe explain how the process went. My first reaction when seeing this was to go to the readme and understand why it's recommended. The readme does not explain anything unfortunately. Only says Without any explanation I started googling around. I could not find any sufficient explanation. Neither on readability nor on performance. The only plus I could find on unpacking, as I mentioned above is you can use it to concatenate mutable and immutable collections, like say list and tuple. With googling inconclusive I wrote a silly test: import random
import timeit
list_1 = random.choices(range(1,1000), k=1000)
print(timeit.timeit(lambda: list_1 + [1, 100]))
print(timeit.timeit(lambda: [*list_1, 1, 100])) Here the first number for me is always smaller, so it does not seem like a good change performance wise. So no readability improvement, no performance improvement, in the end I disabled the rule. But I am still very much intrigued on why people would want this enforced. Do you think it would make sense to have a forum/chat (github has a feature for forums) where we could discuss rules and add hints/tips to make each other's codebase better by explaining these stuff? I think you would not want this in issues as discussion are not actionable and can end up in bike shedding. |
Yeah this is great feedback. We're starting to move in that direction but it's going to take some time to formalize and cover the existing rules. The approach we're taking is based on Clippy, Rust's linter: You can see that we've started adding these to some of the in-code documentation, and eventually those will be surfaced to users.
Yeah this could be a good use-case for forums. Let me think on it for a bit. I want issues to avoid becoming a place where people debate the usefulness of rules, as you've hinted at. For this rule: I'd argue that it increases readability in some cases. But it can also increase consistency -- like, if you use this convention everywhere in your codebases, you get a more consistent codebase vs. sometimes unpacking and sometimes concatenating. But, again, I don't want to take a strong position either way -- I'm just looking to provide an answer on why someone might want to include it. |
Thanks for the answers! Would love to see those take form! Forum could also be per-rule, and have link to the equivalent rule discussion in the readme 😄
That's true but again it's a different kind of tool as you need to use unpacking in some cases as I stated above (concatenation can't work for different types, while unpacking can). So not using the same tool everywhere for this is valid. Anyway I can also see why someone would want it, so no problem. My take-away then for this is that it's a rule that enforces consistency with a small potential performance hint. |
Explaining here: astral-sh/ruff#2054 (comment) Why I don't think that blindly enforcing unpacking over concatenation is a good idea.
Explaining here: astral-sh/ruff#2054 (comment) Why I don't think that blindly enforcing unpacking over concatenation is a good idea.
When running
ruff==0.0.228
on the following code sample via:ruff --select RUF005 .
it suggests:
or auto-fixes it to:
This is unfortunate as those comments are important :D
Thanks so much as always!
The text was updated successfully, but these errors were encountered: