[flake8-builtins] Make strict module name comparison optional (A005)
#15951
Conversation
crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs
Outdated
Show resolved
Hide resolved
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| A005 | 105 | 0 | 105 | 0 | 0 |
|
Have you considered implementing the approach mentioned in #15399 (comment) and if so, why did you decide against it? |
|
I could certainly be missing something here, but from my reading of the comment, this implementation (with the bugfix I just added) should cover that case. Possibly this should be added as a test, but I just added this directrory structure locally: And ran my debug build of ruff: If I enable strict checking, I get 4 additional errors for the Again, I could be missing something, but we don't really have to treat the fully-qualified path or worry about parent-child relationships mentioned in the comment directly because we check each of the files separately. |
crates/ruff_workspace/src/options.rs
Outdated
| default = r#"false"#, | ||
| value_type = "bool", | ||
| example = "builtins-strict-checking = bool" | ||
| )] | ||
| /// Compare module names instead of full module paths. | ||
| pub builtins_strict_checking: Option<bool>, |
There was a problem hiding this comment.
I think we have to keep the existing default and wait for the next minor to change it.
There was a problem hiding this comment.
That makes sense, I'll set the default back to true. Is there a special label I need to apply here or otherwise track that for the next minor release?
There was a problem hiding this comment.
We can change the default for preview. Although I'm not sure if we did that before but it would give us some preview testing.
The easiest for tracking is to create an issue and tag it with the next milestone. Or you can create a follow up PR, mark it as breaking, and add it to the milestone.
| let (module_name, parent) = if is_module_file(path) { | ||
| ( | ||
| package.path().file_name().unwrap().to_string_lossy(), | ||
| package.path().parent(), | ||
| ) | ||
| } else { | ||
| path.file_stem().unwrap().to_string_lossy() | ||
| (path.file_stem().unwrap().to_string_lossy(), path.parent()) | ||
| }; |
There was a problem hiding this comment.
Nit: the only difference between the two branches seems to be whether we call the operations on package.path or path. Could we change the if to either return package.path or path and then extract the module_name and parent outside the if?
There was a problem hiding this comment.
There's also file_name vs file_stem to remove the .py from the non-module file, but this did look a bit better before I made it a tuple. I'll think about an improvement.
crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs
Outdated
Show resolved
Hide resolved
|
I think my example from earlier was actually misleading because I didn't have a Related to your I noticed this when testing a default strict value and not being able to reproduce the eight-error case. |
|
Sorry @MichaReiser, I now believe that you were right all along. I ended up writing a few integration tests with the exact module structure above (including I also changed the default for Once the non-strict behavior becomes default, there will still be a substantial net decrease in violations too. |
crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs
Outdated
Show resolved
Hide resolved
Can you explain in what ways you made the check more strict than it used to be before? I ask because it depends if the changed behavior classifies as a bug fix (fine to do in a patch release) or a behavior change (it depends but generally no) |
crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs
Outdated
Show resolved
Hide resolved
For modules, the original code used The I think we're being more faithful to the
But I'm definitely wary of introducing more diagnostics, especially when the issue I set out to fix was for too many diagnostics. |
|
Thanks again for the feedback here and on Discord! I think I've handled all of the review comments here and also revised the implementation to reflect the upstream behavior better. To help visualize the behavior, here's a table of the strict vs non-strict behavior, operating on the tree from above (again with
Like #15399 (comment), ✅ means we emit a diagnostic and ❌ means no diagnostic. We now match the behavior of the upstream |
|
Thanks for the nice table. I think we should split out the bug fix from the preview behavior change for a clear changelog entry. I hope it isn't too tedious (we can update the changelog manually if it is) |
Sure, no problem! Just to make sure, the bug fix is matching the behavior of upstream (so most of the changes in the rule file, minus the new option), and the preview change is everything to do with the new config option? |
|
Exactly |
See #15951 for the original discussion and reviews. This is just the first half of that PR (reaching parity with `flake8-builtins` without adding any new configuration options) split out for nicer changelog entries.
|
I think I'll have to wait to review this one until your other PR lands. I put it back in draft mode and you can signal that it's ready by marking it ready for review again. |
…16006) See #15951 for the original discussion and reviews. This is just the first half of that PR (reaching parity with `flake8-builtins` without adding any new configuration options) split out for nicer changelog entries. For posterity, here's a script for generating the module structure that was useful for interactive testing and creating the table [here](#15951 (comment)). The results for this branch are the same as the `Strict` column there, as expected. ```shell mkdir abc collections foobar urlparse for i in */ do touch $i/__init__.py done cp -r abc foobar collections/. cp -r abc collections foobar/. touch ruff.toml touch foobar/logging.py ``` --------- Co-authored-by: Micha Reiser <micha@reiser.io>
|
I think this should be ready to review again. It's now only a couple of lines changed in the rule itself, the addition of the config option, and adding the right option value to a couple of existing tests. |
* main: (991 commits) [red-knot] Resolve `Options` to `Settings` (#16000) Bump version to 0.9.6 (#16074) Revert tailwindcss v4 update (#16075) Improve migration document (#16072) Fix reference definition labels for backtick-quoted shortcut links (#16035) RUF009 should behave similar to B008 and ignore attributes with immutable types (#16048) [`pylint`] Also report when the object isn't a literal (`PLE1310`) (#15985) Update Rust crate rustc-hash to v2.1.1 (#16060) Root exclusions in the server to project root (#16043) Directly include `Settings` struct for the server (#16042) Update Rust crate clap to v4.5.28 (#16059) Update Rust crate strum_macros to 0.27.0 (#16065) Update NPM Development dependencies (#16067) Update Rust crate uuid to v1.13.1 (#16066) Update Rust crate strum to 0.27.0 (#16064) Update pre-commit dependencies (#16063) Update dependency ruff to v0.9.5 (#16062) Update Rust crate toml to v0.8.20 (#16061) [`flake8-builtins`] Make strict module name comparison optional (`A005`) (#15951) [`ruff`] Indented form feeds (`RUF054`) (#16049) ...
|
I believe that the documentation for this was not yet updated, right? Was trying to read there what this new setting is without having to go through pages of discussion about it, but dont see it there. |
|
I indeed meant the rule documentation, which is where i (and i guess most people) normally first land when looking up how to change settings regarding a specific rule. |
|
Will do! |
Follow-up to #15951 to update * the options links in A005 to reference `lint.flake8-builtins.builtins-strict-checking` * the description of the rule to explain strict vs non-strict checking * the option documentation to point back to the rule
Summary
This PR adds the configuration option
lint.flake8-builtins.builtins-strict-checking, which is used in A005 to determine whether the fully-qualified module name (relative to the project root or source directories) should be checked instead of just the final component as is currently the case.As discussed in #15399 (comment), the default value of the new option is
falseon preview, so modules likeutils.loggingfrom the initial report are no longer flagged by default. For non-preview the default is still strict checking.Test Plan
New A005 test module with the structure reported in #15399.
Fixes #15399