-
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 #2142
Comments
Thanks for bringing to my attention. It's not the first time that we've received some feedback on RUF005 (more here). I worry that the rule is too opinionated to be enabled without some form of explicit opt-in. One solution here would be to move that check out of Another would to remove it entirely (in a backwards-compatible way, with a warning, etc.). |
\cc @LefterisJP |
I think the other comment I'll make here is that I don't anticipate Ruff continually adding "opinionated" checks. Including that one was, perhaps, a mistake. I'd prefer for Ruff to focus on enforcing correctness. The |
I'll also \cc @akx who implemented the rule. (Not to call you out or ask you to justify it -- it's ultimately my call as to what gets included and what doesn't -- but just so you have a chance to chime in if you'd like :)) |
Thanks for the quick response! Our hurdle was that we encountered this because we have |
Totally. I empathize with a lot of what's being said in that thread. I definitely don't want the pace of Ruff's development to be creating an experience for users whereby they feel they're in constant churn as new rules are added. (This is kind of the risk with Another thing that's going to help here is the introduction of autofix levels. So we'll let be able to mark rules by the degree of "safety" / "correctness", and allow users to turn off autofixes that could be incorrect. |
Yeah @nstarman I agree with the reluctancy to apply it to a codebase as linked by Charlie above here is my comment: #2054 (comment) But find it nice still to see those changes, and play with them one by one when testing a new ruff release and figuring out what to keep and what to ignore in our project's settings. |
We probably need to mark rules with more details as discussed in #2054. Such as rule A is mainly for consistency, but may reduce performance. Rule B is a no-brainer, consistency and performance. Rule C is a stylistic choice (would need lots of those rules for customizable auto-formatting direction you wanna go). So people can easily check when upgrading instead of running timeit tests or googling. And definitely in that detailed explanation have a description as to the thinking behind the rule. |
Hey, thanks @charliermarsh for cc'ing me in, and sorry for the added churn in astropy, @nstarman et al :) For RUF005, I deliberately worded the message as "consider ..." instead of "do this" because it's a bit case-dependent, and as discussed above, the autofix for it should probably be categorized "non-mandatory". However, looking at diffs like available = tuple(sorted(p.stem for p in _COSMOLOGY_DATA_DIR.glob("*.ecsv")))
-__all__ = ["available", "default_cosmology"] + list(available)
+__all__ = ["available", "default_cosmology"]
+__all__ += list(available) in the linked Astropy PR, that looks like it should be available = tuple(sorted(p.stem for p in _COSMOLOGY_DATA_DIR.glob("*.ecsv")))
-__all__ = ["available", "default_cosmology"] + list(available)
+__all__ = ["available", "default_cosmology", *available] – one big benefit of splatting like that is avoiding going extra calls to On that note, @charliermarsh, it could be pretty cool if "suggestion" fixes could show the actual fix diff inline in the CLI output 🤔, á la
EDIT: I guess |
Just wanted to add to this discussion that
but [*np.arange(3), 1, 1, 1]
>>> [0, 1, 2, 1, 1, 1] |
I found another problem with RUF005. It converts: import numpy as np
array = np.array([[1, 2, 3], [4,5,6]])
print(array + [5,4]) into import numpy as np
array = np.array([[1, 2, 3], [4,5,6]])
print([*array, 5, 4]) Which are not equivalent codes. I do not have any idea if it is possible to determine if the first argument is a list or numpy array. (if It is required I may move it to a separate Issue). |
Ah yeah, that's hard to detect right now. I'd recommend turning off the rule entirely if you're using |
I wonder if we could be smart about that and disable it for now if the module imports |
Closing for now as I don't think there's anything actionable here. |
We're having a lively debate at astropy/astropy#14297 and found that RUF005 may not deliver the advertised speed boost.
The text was updated successfully, but these errors were encountered: