RUF005

[nstarman]

We're having a lively debate at astropy/astropy#14297 and found that RUF005 may not deliver the advertised speed boost.

[charliermarsh]

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 RUF, and into some category that's widely advertised as requiring explicit opt-in (like XXX, which I don't like, but just trying to convey the point). Users would be required to turns these rules on individually (so --select XXX wouldn't work, and if we add more rules to XXX, they won't be turned on when you upgrade -- you'd have to opt-in). Part of the tension here is that it's not really Ruff's intent to tell you that you should enable this check, only that it's available. (The RUF rules suffer most here because they haven't been categorized yet, and so users are kind of opting into a grab-bag of checks. If that rule was in SIM, it might be less controversial, since all of the simplifications in SIM are subjective.)

Another would to remove it entirely (in a backwards-compatible way, with a warning, etc.).

[charliermarsh]

\cc @LefterisJP

[charliermarsh]

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 flake8-simplify checks are kind of borderline.

[charliermarsh]

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 :))

[nstarman]

Thanks for the quick response! Our hurdle was that we encountered this because we have ALL turned on (given the pace of improvement of Ruff it's easy to miss new rules). Personally, I don't mind opinionated rules, but if these are added, perhaps there should be 2 levels of ALL, including (or not) the opinionated rules.

[charliermarsh]

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 ALL. It's also a little bit of a risk with other categories, since we could in theory add new rules to flake8-simplify -- but those categories are bounded by the rules implemented in the originating plugin. We don't add rules to SIM that don't exist in flake8-simplify. So at least there are some guardrails...)

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.

[LefterisJP]

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.

[LefterisJP]

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.

[akx]

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 list and tuple – so I feel the "consider" wording is appropriate to try and direct the reader to read the suggestion and surrounding code to see if it could be done better...

On that note, @charliermarsh, it could be pretty cool if "suggestion" fixes could show the actual fix diff inline in the CLI output 🤔, á la

RUF005: consider using ...
  - ["available", "default_cosmology"] + foo + bar
  + ["available", "default_cosmology", *foo, *bar]

EDIT: I guess --show-source does that!

[janosh]

Just wanted to add to this discussion that RUF005 is a bit too eager atm when it comes to numpy arrays.

import numpy as np

np.arange(3) + [1,1,1]
>>> [1, 2, 3]

but RUF005 turns this into

[*np.arange(3), 1, 1, 1]
>>> [0, 1, 2, 1, 1, 1]

[Czaki]

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).

[charliermarsh]

Ah yeah, that's hard to detect right now. I'd recommend turning off the rule entirely if you're using numpy.

[akx]

Ah yeah, that's hard to detect right now. I'd recommend turning off the rule entirely if you're using numpy.

I wonder if we could be smart about that and disable it for now if the module imports numpy... That's of course not quite bullet-proof. I don't know if there's been discussion about a "confidence"/"severity" system for Ruff (or if that's way extra), but RUF005 could be a low-confidence lint...

[charliermarsh]

Closing for now as I don't think there's anything actionable here.