[flake8-use-pathlib] Make PTH100 fix unsafe because it can change behavior#20100
[flake8-use-pathlib] Make PTH100 fix unsafe because it can change behavior#20100ntBre merged 3 commits intoastral-sh:mainfrom
flake8-use-pathlib] Make PTH100 fix unsafe because it can change behavior#20100Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PTH100 | 208 | 0 | 0 | 0 | 208 |
ntBre
left a comment
There was a problem hiding this comment.
Thanks! Just a couple of small suggestions.
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_abspath.rs
Outdated
Show resolved
Hide resolved
| /// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression. | ||
| /// | ||
| /// `Path.resolve()` resolves symlinks, while `os.path.abspath()` does not. | ||
| /// If preserving symlinks is important, you may need to use `Path.absolute()`. | ||
| /// The suggested autofix is marked as unsafe because it can change behavior. |
There was a problem hiding this comment.
I don't think it's so important to mention the comment issue now that the rule is always unsafe. I'd probably say something like:
This rule's fix is always marked as unsafe because
Path.resolve()resolves symlinks, whileos.path.abspath()does not. If resolving symlinks is important, you may need to usePath.absolute(). However,Path.absolute()also does not remove any..components in a path, unlikeos.path.abspath()andPath.resolve(), so if that specific combination of behaviors is required, there's no existingpathlibalternative. See CPython issue #69200.
That's a bit wordy, so it could be nice to include @dscorbett's nice table from the issue instead, up to you.
There was a problem hiding this comment.
I can't comment on the line below, but as I mentioned on the issue, we should also update the Correspondence between os and pathlib link below (#20088 (comment)). The new link is https://docs.python.org/3/library/pathlib.html#corresponding-tools
There was a problem hiding this comment.
It seems to me that if a fix is going to change behavior in any way, it's better to leave it as a unsafe
|
Should I change links in this PR in the same way in all |
|
I also see that there are many more functions in the table for which there are no rules and fixes, can we start adding them? |
Sure, but let's do that in a follow-up documentation PR. I think this one is good to go now.
I think we've implemented all of the rules in the upstream |
…ndence between `os` and `pathlib` (#20103) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary Part of #20100 | #20100 (comment)
* main: [`ruff`] Preserve relative whitespace in multi-line expressions (`RUF033`) (astral-sh#19647) [ty] Optimize TDD atom ordering (astral-sh#20098) [`airflow`] Extend `AIR311` and `AIR312` rules (astral-sh#20082) [ty] Preserve qualifiers when accessing attributes on unions/intersections (astral-sh#20114) [ty] Fix the inferred interface of specialized generic protocols (astral-sh#19866) [ty] Infer slightly more precise types for comprehensions (astral-sh#20111) [ty] Add more tests for protocols (astral-sh#20095) [ty] don't eagerly unpack aliases in user-authored unions (astral-sh#20055) [`flake8-use-pathlib`] Update links to the table showing the correspondence between `os` and `pathlib` (astral-sh#20103) [`flake8-use-pathlib`] Make `PTH100` fix unsafe because it can change behavior (astral-sh#20100) [`flake8-use-pathlib`] Delete unused `Rule::OsSymlink` enabled check (astral-sh#20099) [ty] Add search paths info to unresolved import diagnostics (astral-sh#20040) [`flake8-logging-format`] Add auto-fix for f-string logging calls (`G004`) (astral-sh#19303) Add a `ScopeKind` for the `__class__` cell (astral-sh#20048) Fix incorrect D413 links in docstrings convention FAQ (astral-sh#20089) [ty] Refactor inlay hints structure to use separate parts (astral-sh#20052)
… behavior (astral-sh#20100) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary Fixes astral-sh#20088 <!-- What's the purpose of the change? What does it do, and why? --> ## Test Plan `cargo nextest run flake8_use_pathlib` --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
…ndence between `os` and `pathlib` (astral-sh#20103) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary Part of astral-sh#20100 | astral-sh#20100 (comment)
Summary
Fixes #20088
Test Plan
cargo nextest run flake8_use_pathlib