[magic-value-comparison] Custom allowlist for Magic Numbers (PLR2004)#18961
[magic-value-comparison] Custom allowlist for Magic Numbers (PLR2004)#18961DaniBodor wants to merge 18 commits intoastral-sh:mainfrom
Conversation
9ec2b5a to
3664fae
Compare
|
|
@DaniBodor any chance you could look at the failing tests so that we could move this PR across the line? |
|
Yes.. I'm sorry, I haven't had much time recently. Will pick it up within the next couple of weeks. |
|
Is there any update on this? 🙂 |
|
Hi. I'm sorry for the radio silence. 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. |
2c8bf02 to
d8d9d7a
Compare
|
Apologies again for leaving this open for so long. |
|
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 :) |
instead of try_from_literal_expr
i.e. listing an allowed magic value of the float `1.0` will also accept the int `1` (and vice versa)
|
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. |
|
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 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. |
36ea89a to
80dd15b
Compare
Yup, that was the issue. When leaving the conda env the tests passed fine without updating ty snapshots. |
|
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. |
in addition to defining custom list (which replaces the defaults)
80dd15b to
072f99b
Compare
|
@ntBre just checking whether this is still on your radar or whether it fell through the cracks since resolving the ty issues. |
|
This is still on my list to review! |
just a friendly ping because it's been a few weeks now |
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this looks pretty reasonable to me at a high level. I had a few suggestions for simplifying a bit further, and I think we should add some tests. At a minimum we should test the somewhat tricky logic in AllowedValue::matches with tests in crates/ruff_linter/src/rules/pylint/mod.rs, but we may also want to include a CLI test or two for parsing from an actual TOML file.
crates/ruff_linter/src/rules/pylint/rules/magic_value_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/magic_value_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/magic_value_comparison.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
DaniBodor
left a comment
There was a problem hiding this comment.
Thanks for the review!
Sorry for initially commenting on individual points, rather than batching them in one review.
I think tests make a lot of sense, I use a dummy toml file for one of the repos I used to work on as a test. In doing so, I actually discovered that there's a bug with negative values, which I will need to look into.
I'll also look into creating tests, but may need some help if I get stuck.
In the meantime, I think I've addressed all your other comments already. I will wait with re-opening this PR until I have finished some tests and fixing that negative value bug, but in principle am not working anymore your other comments.
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 settingpylint.allow_magic_value_typesThe 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 does is added in addition to thepylint.allow_magic_value_typesand 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 during development, but I definitely relied heavily on this to solve some of the issues I ran into during development.
fix: #10009