Implement PLR2004 (MagicValueComparison)

[max0x53]

This PR adds Pylint R2004

Feel free to suggest changes and additions, I have tried to maintain parity with the Pylint implementation magic_value.py

See #970

[charliermarsh]

This looks great! Couple comments to get it fully aligned with the Pylint implementation.

Speaking of: how do you enable this check with Pylint? I notice it's part of an "extension". Trying to figure out how to test it locally...

[max0x53]

To test R2004 with pylint you will need to follow their instructions for Contributor installation.

Once the basic installation is complete you can run the following to confirm you are on the main branch.

$ pylint --version
pylint 2.16.0-dev

Once installed you can run the following command to test R2004 (with the flag documented here)

$ pylint --load-plugins=pylint.extensions.magic_value ./magic_value_comparison.py | grep R2004
<PATH>/ruff/resources/test/fixtures/pylint/magic_value_comparison.py:6:3: R2004: Consider using a named constant or an enum instead of '10'. (magic-value-comparison)
<PATH>/ruff/resources/test/fixtures/pylint/magic_value_comparison.py:29:3: R2004: Consider using a named constant or an enum instead of '2'. (magic-value-comparison)
<PATH>/ruff/resources/test/fixtures/pylint/magic_value_comparison.py:44:3: R2004: Consider using a named constant or an enum instead of 'Hunter2'. (magic-value-comparison)
<PATH>/ruff/resources/test/fixtures/pylint/magic_value_comparison.py:50:3: R2004: Consider using a named constant or an enum instead of '3.141592653589793'. (magic-value-comparison)
<PATH>/ruff/resources/test/fixtures/pylint/magic_value_comparison.py:59:3: R2004: Consider using a named constant or an enum instead of 'b'something''. (magic-value-comparison)

Hope this helps!

Current ruff output for completeness

resources/test/fixtures/pylint/magic_value_comparison.py:6:4: PLR2004 Magic number used in comparison, consider replacing 10 with a constant variable
resources/test/fixtures/pylint/magic_value_comparison.py:29:4: PLR2004 Magic number used in comparison, consider replacing 2 with a constant variable
resources/test/fixtures/pylint/magic_value_comparison.py:44:4: PLR2004 Magic number used in comparison, consider replacing 'Hunter2' with a constant variable
resources/test/fixtures/pylint/magic_value_comparison.py:50:4: PLR2004 Magic number used in comparison, consider replacing 3.141592653589793 with a constant variable
resources/test/fixtures/pylint/magic_value_comparison.py:59:4: PLR2004 Magic number used in comparison, consider replacing b'something' with a constant variable
Found 5 error(s).

[charliermarsh]

Oh, thank you! So helpful.

I think there are still a few false-positives here:

if x == 3:
    pass

if 1 == 3:
    pass

if 4 == 3 == x:
    pass

Ruff gives:

❯ cargo run foo.py --select PLR2004 -n
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/ruff foo.py --select PLR2004 -n`
foo.py:1:4: PLR2004 Magic number used in comparison, consider replacing 3 with a constant variable
foo.py:4:4: PLR2004 Magic number used in comparison, consider replacing 3 with a constant variable
foo.py:7:4: PLR2004 Magic number used in comparison, consider replacing 3 with a constant variable
foo.py:7:4: PLR2004 Magic number used in comparison, consider replacing 4 with a constant variable
Found 4 error(s).

Whereas Pylint gives:

❯ pylint --load-plugins=pylint.extensions.magic_value foo.py | grep R2004
foo.py:1:3: R2004: Consider using a named constant or an enum instead of '3'. (magic-value-comparison)

I think this comes down to: (1) for the purpose of skipping, Pylint considers 1 and such to be constants, even if they don't get flagged as magic, whereas the current implementation here does not (since it relies on the number of diagnostics generated); and (2) it looks like Pylint actually skips if there's any constant comparison, not just if they're all constant.

[max0x53]

Let me know if you would prefer the 2nd for loop to be refactored into an iterator, have left as is for now.

[charliermarsh]

Thank you! Running CI now. Will merge on pass.

[max0x53]

Cheers! Appreciate you being so responsive to PRs and a credit to ruff for being as easy as it was to add new linting rules (docs and code were great)!

[charliermarsh]

Thank you, that's awesome to hear! (\cc @not-my-profile as he's made some refactors lately that, IMO, made adding rules a lot more accessible.)

[martinkozle]

Now how do I ignore this lint? Because when I put ignore = ["PLR2004"] in pyproject.toml it says that it is not a valid option.

Edit: nvm, it does have an effect, Better TOML VSCode extension just says that it is not a valid option, I don't know if the problem is on my end

Edit 2: The linting was based on the version of ruff that comes with the VSCode extension where this lint wasn't implemented yet. So yes, it was a problem on my end

[charliermarsh]

(We have to update the JSON Schema manually in the Schema Store, unfortunately, so it sometimes gets a little out-of-date with latest Ruff.)

[ofek]

Feature request: #1949