Consider splitting PLR2004 (MagicValueComparison) into different rules

[ofek]

I would suggest one rule for numeric types and another that would be the current behavior. Checking string values inline is extremely common and as it is now when I have time I'm going to disable this for new projects generated by Hatch.

[ofek]

For example, Hatch which is a moderately sized project with high code coverage has almost 600 instances of this and none of them are numeric https://github.com/pypa/hatch/actions/runs/3946429939/jobs/6754217718 😅

[martinkozle]

Yeah this rule doesn't make sense to me for single occurrence inline strings. Inline string values most of the time are self explanatory because, well, they are strings. And in 90% of cases the constant variable that would replace them would have the same exact name as the string value.
Is it ruff's goal to be opinionated and make such changes? Because this is just an implementation of pylint rule R2004. This issue should probably be reopened on the pylint repository if I am not mistaken.

[charliermarsh]

In this case, it's Ruff's goal for R2004 to be a faithful reimplementation of the Pylint rule. We do deviate from "upstream" rules to fix clear false positives or false negatives (especially when there are open issues in the upstream repo), but in this case, it feels like this would be changing the definition of the rule?

(Though it's outside the scope of this issue: eventually, we want to get to that point that the flake8 and pylint terminology and mappings are viewed as more of a compatibility layer. E.g., we'll likely recategorize the rules into a first-class taxonomy; we may migrate from numeric to human-readable codes, and so on. Should we make these changes, the intent is for it be done in a way that preserves existing compatibility and even allows for "automatic" configuration migrations -- but the idea is that we'd start viewing Ruff's taxonomy as its own standalone thing rather than a reimplementation, while retaining all the compatibility benefits we get today.)

A few comments (even if not a prescriptive solution, yet -- to be clear, I definitely want to figure out something that works for @ofek here):

• We could introduce a separate rule in Ruff itself (in the RUF category` that's more discriminate, and Hatch (and others) could use that in lieu of the Pylint rules.
• We could add a configuration option to avoid checking strings. That doesn't exist in Pylint, but it'd be easy to add (I could do it today, certainly).
• This rule is actually only enabled in Pylint when users turn on the Magic-value extension. So in some sense, it should require more "opt-in" than merely enabling the PLR rule category. We don't have a great way to express that in our API, but I thought I would mention it.

[charliermarsh]

Specifically, a configuration option would look something like:

[tool.ruff.pylint]
allow-magic-value-comparisons = ["str"] # Or "string"? Or "String"?

[ofek]

Yeah that sounds like a reasonable option. In the future scenario in which Ruff has its own rule taxonomy it would be nice if the default behavior excluded strings since almost everyone will add that to their config, or simply not use PL*

[charliermarsh]

Yeah, I would likely either exclude strings or even omit this rule altogether from the default set. (I personally don't find it useful, but I try to avoid injecting too much of my own opinion when porting third-party rules.)

[charliermarsh]

I do think it'd be reasonable for you to omit the PL* rules by default in Hatch.

[ofek]

I would likely either exclude strings or even omit this rule altogether from the default set.

I think the rule for numeric types is quite useful and is actually something that I will probably always comment on in a code review.

I do think it'd be reasonable for you to omit the PL* rules by default in Hatch.

I hadn't considered doing that but maybe that's a good idea. 2004 is the only thing that I think would be disruptive though and I want beginners to get as much benefit as possible out of this in order to learn best practices.

[charliermarsh]

Added said option in #1987. I actually went ahead and ignored string comparisons by default per this thread. It deviates from Pylint, but users can always re-enable string comparisons via the setting, so that's fine with me.

[ofek]

Thank you!!!

[charliermarsh]

Of course, thanks as always for being such a great supporter of the project @ofek!