Harmonise methods for distinguishing different Python source types #13682
Conversation
AlexWaygood
force-pushed
the
alex/remove-redundant-routines
branch
from
bd426ea to
6251505
Compare
| } else if Path::new(url.path()) | ||
| .extension() | ||
| .map_or(false, |ext| ext.eq_ignore_ascii_case("ipynb")) | ||
| { | ||
| } else if PySourceType::from(url.path()).is_ipynb() { |
There was a problem hiding this comment.
this compares case-sensitively where the existing code compares case-insensitively. I don't actually know if that's correct for Jupyter notebooks, but I do know that casing does matter for other Python files. import foo succeeds if the foo module is found at foo.py, but fails if the foo module is found at foo.PY
There was a problem hiding this comment.
Seems overkill to make the breaking change for ipynb, I think the .PY import logic does make sense though?
There was a problem hiding this comment.
this compares case-sensitively where the existing code compares case-insensitively. I don't actually know if that's correct for Jupyter notebooks, but I do know that casing does matter for other Python files.
import foosucceeds if thefoomodule is found atfoo.py, but fails if thefoomodule is found atfoo.PY
Is this also true on case insensitive file systems? I do think that import resolver is different from what we consider a python file
There was a problem hiding this comment.
My experience is that most applications don't consider file extensions to be case sensitive. Naming a file test.txt or test.TXT doesn't change the application in which my desktop environment opens the file. That's why I think we shouldn't treat file extensions as case sensitive but I can see how this is beyond the scope of this pr
It does make sense that the module resolver only tests for the existence of ’.py’ and whether ’.PY’ is considered to match depends on the file system's case sensitivity
There was a problem hiding this comment.
For now I will revert these changes so that this PR is a "pure refactor" that does not change functionality. I think we may need to review this, though. In most places, we currently do case-sensitive matching for file extensions. Maybe that's correct or maybe it's incorrect, but it seems like we're pretty inconsistent right now. (And if that inconsistency is correct, because we need to apply logic in different situations, then we could at least add some comments to explain it more clearly 😄)
|
crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_module_shadowing.rs
Outdated
Show resolved
Hide resolved
|
Could you create an issue to re-visit the case-insensitive extension matching and assign it to the next milestone. We should re visit this soon. |
CodSpeed Performance ReportMerging #13682 will degrade performances by 4.13%Comparing Summary
Benchmarks breakdown
|
|
the codspeed flamegraph for this PR is very noisy so it's hard to tell, but I think that's just a false positive. I don't think I can see anything that would be plausibly related to this PR. |
I opened #13691 |
Summary
I was looking into #13246 to see if we could land the proposed change as part of Ruff 0.7, and noticed that we're currently pretty inconsistent in how we try to determine whether something is a Python source file or not (and, if so, what kind of source file it is). This PR tries to make it so that we only ever use
ruff_python_ast::PySourceTypefor that purpose. This removes duplicated logic and the number of magic"py"or"pyi"strings across the codebase, and it should make it easier to make changes such as #13246 in the future.Test Plan
cargo test