[DaniBodor]

Summary

This PR allows users to specify a custom set of whitelisted values for magic value comparisons in PLR2004. The setting is named pylint.allow_magic_values, which is consistent with a similar setting pylint.allow_magic_value_types
The previous hard-coded allow-list was moved to the default for pylint.allow_magic_values, giving users full control over what is and what is not a whitelisted value. This setting is added in addition to the pylint.allow_magic_value_types and does not replace it. Any value that is whitelisted due to either of these settings is not considered a linting violation.

Note that as far as I could tell, no other setting allows floats or complex numbers as inputs, so I created a few helper methods to deal with these properly (mainly relating to floating point variation).

Test Plan

I have tested the settings with a number of cases manually in a repository I work on regularly. I have not added any extensive (combinatorial) tests to the test suite.

Note

I don't know whether you have any policy on using or disclosing the use of code generators, but I definitely relied heavily on this to solve some of the issues I ran into during development. Ultimately, all code was reviewed by me before pushing.

fix: #10009

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

@DaniBodor any chance you could look at the failing tests so that we could move this PR across the line?

[DaniBodor]

Yes.. I'm sorry, I haven't had much time recently. Will pick it up within the next couple of weeks.

[marciska]

Is there any update on this? 🙂

[DaniBodor]

Hi. I'm sorry for the radio silence.
Due to life events, I have not been able to work on this in quite a while.

I now have some limited availability again, so can start picking this up again, but am hesitant to bite off more than I can chew atm, so I may need some help moving this forward.

[DaniBodor]

Apologies again for leaving this open for so long.
Given the time delay, some conflicts with main appeared. I've now rebased this off the new main and addressed two out of three points raised by @MichaReiser, and resolved all the issues with failing checks.
I can now start looking into the remaining comment as well.

[ntBre]

No problem at all! Thanks for rebasing and for your continued work on this. Will you re-request my review when you're ready? Micha is out on vacation, but I'm happy to review :)

[ntBre]

I think something may have gone slightly awry with a merge or rebase. I'm seeing some modified ty files, which also prompted the code owner review requests. Let me know if you need any help sorting that out!

[DaniBodor]

I think something may have gone slightly awry with a merge or rebase. I'm seeing some modified ty files, which also prompted the code owner review requests. Let me know if you need any help sorting that out!

Help would be appreciated.
The files in the last commit (which added the ty files) got auto-generated/-edited by running the test suite, as instructed to do in the contribution instructions. Without that, the tests fail locally for me.

[ntBre]

Huh, that's strange. It doesn't look like a merge issue to me. Are you running the tests inside of a virtual environment by any chance? Maybe even a conda environment? I wonder if that is leaking into the tests. I'm mostly guessing since it looks like additional LSP diagnostics are showing up offering to add imports like import anaconda_cli_base.deprecations.deprecated.

I can certainly update the snapshots on my computer to pass CI, but I expect you'll run into the same problem on future iterations, so it may be worth tracking down.

[DaniBodor]

Are you running the tests inside of a virtual environment by any chance? Maybe even a conda environment? I wonder if that is leaking into the tests. I'm mostly guessing since it looks like additional LSP diagnostics are showing up offering to add imports like import anaconda_cli_base.deprecations.deprecated.

Yup, that was the issue. When leaving the conda env the tests passed fine without updating ty snapshots.

[DaniBodor]

Getting back to the content of this PR:

I've addressed all the issues raised in the previous review. I've also added 2 additional features to this setting, that were not part of the original PR.
First, separate options an option to extend the default allow list or to create a fully custom allow list (or just use the default, obviously) - 072f99b.
Second (this one I am not positive is actually wanted, so please tell me your thoughts), cross recognizing of whole-number floats and integers. By this I mean that if for example the float 2.0 is on the allow list then the int 2 will also be accepted, and vice versa - 82daee0.

[astral-sh-bot]

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 96.96%. The percentage of expected errors that received a diagnostic held steady at 92.96%. The number of fully passing files held steady at 106/133.

[astral-sh-bot]

Memory usage report

Memory usage unchanged ✅

[astral-sh-bot]

ecosystem-analyzer results

No diagnostic changes detected ✅

Flaky changes detected. This PR summary excludes flaky changes; see the HTML report for details.

Full report with detailed diff (timing results)

[DaniBodor]

I'm leaning toward avoiding the complexity of the numeric comparisons for now. See my inline comment for a little more detail.

Hi @ntBre ,
I finally had some time on my hands once again to look back into this. Given that there's been quite some changes to ruff since my last edits, I decided to rebase the entire branch off of the most recent main ruff branch. I had to do some manual conflict fixes, but am reasonably confident it all went ok.

I agree that cross-checking ints against floats and vice versa probably adds excessive complexity for how useful it is. In fact, I could even imagine cases where a user does want to white-list the one but not the other. So indeed probably best to remove it. I reverted the the code that does the cross-checking in the last few commits.

When pushing the newest changes, I keep running into failed CI checks for which I can't figure out what is wrong. Locally, when I run any of the diagnostics listed in the dev documentation it passes just fine.
Any chance you can help me diagnose this? FYI, 588afd4 was added in response to this failed check on the previous commit, but seems to have auto-triggered additional reviewers to this PR (once again).

[ntBre]

Thanks for the update!

The CI failure I'm seeing is for ruff_dev::bin/ruff_dev generate_json_schema::tests::test_generate_json_schema, which means you need to run the cargo dev generate-all command suggested (way earlier 😅) in the output. The suggestion to run cargo insta test --force-update-snapshots is to fix a warning that won't break CI, so we should drop that commit for now.

The contributing docs you linked to are for ty. This is probably the better reference for Ruff, including the mention of cargo dev generate-all: https://github.com/astral-sh/ruff/blob/main/CONTRIBUTING.md#example-adding-a-new-configuration-option.

[DaniBodor]

Thanks for the update!

The CI failure I'm seeing is for ruff_dev::bin/ruff_dev generate_json_schema::tests::test_generate_json_schema, which means you need to run the cargo dev generate-all command suggested (way earlier 😅) in the output. The suggestion to run cargo insta test --force-update-snapshots is to fix a warning that won't break CI, so we should drop that commit for now.

The contributing docs you linked to are for ty. This is probably the better reference for Ruff, including the mention of cargo dev generate-all: https://github.com/astral-sh/ruff/blob/main/CONTRIBUTING.md#example-adding-a-new-configuration-option.

Thanks for pointing me to the correct contribution guidelines, not sure how I ended up on the ty guide, it's been the one I was using from the start, which explains why similar things happened a few time 🤦.

I now dumped the force-update-snapshots commit and instead ran the json schema update and that was indeed all that was required to make the tests pass.
This PR is now ready for review again

[DaniBodor]

There was a comment on the original issue regarding the name of these rules that hasn't been addressed in this PR or the discussion yet.

#10009 (comment) says:

If the rule's named magic-value-comparison (PLR2004) then an option lint.pylint.allow-magic-values seems appropriate.
There's already lint.pylint.allow-magic-value-types, so "allow" rather than "allowed" and "values" rather than "value" would keep a similar pattern.

Although that does bring into question whether lint.pylint.allow-magic-value-types and lint.pylint.allow-dunder-method-names should have been called allowed-* because it's a list of options, not a boolean. As the pattern other rule options seem to follow.

I agree that using allowed-* for all these settings is semantically better, but I guess the question is whether it is worth creating a breaking change with the previous existing rule-name.

EDIT:
Another point that hasn't been addressed (yet) from the issue that this is based off, is whether 2 and 3 should be part of the default allowed list. This was discussed prior to proposing to create an adaptable setting. I think that it would make sense to add 2, but not 3. I won't touch the PR now, in case that would make it move down your to do list, but if you agree I'll add it in the next round of changes.

[DaniBodor]

@ntBre , just checking in as it's been 2 months since the last updates to this PR and I'm waiting for a review. I have the feeling this PR is close to complete, but new conflicts keep popping up so it would be nice to try and finalize it :)

[ntBre]

Thank you, and sorry for the delay! This looks good to me. I pushed one commit adding a few tests and making a couple of minor simplifications.