[flake8-use-pathlib] Add autofix for PTH101, PTH104, PTH105, PTH121

[chirizxc]

Summary

Part of #2331

Test Plan

cargo nextest run flake8_use_pathlib

[chirizxc]

sorry for #19298 😕

[ntBre]

Do you know what happened on the other PR? I just saw something similar on #19338.

I'll try to take a look soon, but it might not be until next week. Thanks for working on these!

[chirizxc]

Do you know what happened on the other PR? I just saw something similar on #19338.

I'll try to take a look soon, but it might not be until next week. Thanks for working on these!

it was my mistake

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +48 -0 fixes in 3 projects; 52 projects unchanged)

apache/airflow (+0 -0 violations, +30 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ airflow-core/src/airflow/configuration.py:2131:9: PTH101 [*] `os.chmod()` should be replaced by `Path.chmod()`
- airflow-core/src/airflow/configuration.py:2131:9: PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+ airflow-core/src/airflow/dag_processing/bundles/base.py:391:13: PTH105 [*] `os.replace()` should be replaced by `Path.replace()`
- airflow-core/src/airflow/dag_processing/bundles/base.py:391:13: PTH105 `os.replace()` should be replaced by `Path.replace()`
+ airflow-core/src/airflow/utils/log/file_task_handler.py:841:17: PTH101 [*] `os.chmod()` should be replaced by `Path.chmod()`
- airflow-core/src/airflow/utils/log/file_task_handler.py:841:17: PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+ airflow-core/tests/unit/core/test_impersonation_tests.py:71:17: PTH101 [*] `os.chmod()` should be replaced by `Path.chmod()`
- airflow-core/tests/unit/core/test_impersonation_tests.py:71:17: PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+ airflow-core/tests/unit/core/test_impersonation_tests.py:80:21: PTH101 [*] `os.chmod()` should be replaced by `Path.chmod()`
- airflow-core/tests/unit/core/test_impersonation_tests.py:80:21: PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+ airflow-core/tests/unit/core/test_impersonation_tests.py:87:13: PTH101 [*] `os.chmod()` should be replaced by `Path.chmod()`
- airflow-core/tests/unit/core/test_impersonation_tests.py:87:13: PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+ dev/breeze/src/airflow_breeze/utils/kubernetes_utils.py:104:13: PTH101 [*] `os.chmod()` should be replaced by `Path.chmod()`
... 14 additional changes omitted for rule PTH101
+ dev/breeze/src/airflow_breeze/utils/reproducible.py:144:9: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- dev/breeze/src/airflow_breeze/utils/reproducible.py:144:9: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ scripts/in_container/run_migration_reference.py:212:9: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- scripts/in_container/run_migration_reference.py:212:9: PTH104 `os.rename()` should be replaced by `Path.rename()`

latchbio/latch (+0 -0 violations, +2 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ src/latch_cli/utils/__init__.py:278:9: PTH101 [*] `os.chmod()` should be replaced by `Path.chmod()`
- src/latch_cli/utils/__init__.py:278:9: PTH101 `os.chmod()` should be replaced by `Path.chmod()`

zulip/zulip (+0 -0 violations, +16 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ scripts/lib/zulip_tools.py:60:9: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- scripts/lib/zulip_tools.py:60:9: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ scripts/lib/zulip_tools.py:730:5: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- scripts/lib/zulip_tools.py:730:5: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ tools/lib/provision_inner.py:158:9: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- tools/lib/provision_inner.py:158:9: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ zerver/management/commands/export_usermessage_batch.py:46:17: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- zerver/management/commands/export_usermessage_batch.py:46:17: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ zerver/management/commands/export_usermessage_batch.py:60:17: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- zerver/management/commands/export_usermessage_batch.py:60:17: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ zerver/management/commands/fetch_tor_exit_nodes.py:71:9: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- zerver/management/commands/fetch_tor_exit_nodes.py:71:9: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ zerver/tornado/event_queue.py:714:9: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- zerver/tornado/event_queue.py:714:9: PTH104 `os.rename()` should be replaced by `Path.rename()`
+ zerver/worker/base.py:151:13: PTH104 [*] `os.rename()` should be replaced by `Path.rename()`
- zerver/worker/base.py:151:13: PTH104 `os.rename()` should be replaced by `Path.rename()`

Changes by rule (3 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PTH101	26	0	0	26	0
PTH104	20	0	0	20	0
PTH105	2	0	0	2	0

[StDymphna]

@chirizxc
Since you've been implementing autofixes for PTH* rules, are you planning on implementing autofixes for FURB101 and FURB103 (which are PTH related)?

[chirizxc]

@chirizxc
Since you've been implementing autofixes for PTH* rules, are you planning on implementing autofixes for FURB101 and FURB103 (which are PTH related)?

I think so, but I won't promise😴

[ntBre]

The implementation looked good to me, but do you know what happened with the missing snapshots? It looks like we lost a bunch of violations even though their Python files aren't modified. It might be a minor issue with the is_rule_enabled checks, hopefully.

[chirizxc]

The implementation looked good to me, but do you know what happened with the missing snapshots? It looks like we lost a bunch of violations even though their Python files aren't modified. It might be a minor issue with the is_rule_enabled checks, hopefully.

Initial tests in the files are not correct

[chirizxc]

These tests were taken from the original flake8-use-pathlib repository, now if you try to run these tests you will get Missing argument, since they require two arguments in general

[ntBre]

Ohhh, they have the wrong number of arguments? Okay, this is probably good to go then. I'll take one more look at the snapshots. Thank you for clarifying and for fixing that pre-existing bug!

[ntBre]

Hmm, on second thought, maybe we still want diagnostics in theses cases and just to suppress the fixes. What do you think about that? I think the diagnostics are still useful, but fixing could be dangerous since there's another error there already.

[chirizxc]

I think it's a bit weird to suggest diagnostic when the code can't run due to an error

[chirizxc]

Hmm, on second thought, maybe we still want diagnostics in theses cases and just to suppress the fixes. What do you think about that? I think the diagnostics are still useful, but fixing could be dangerous since there's another error there already.

in some rules it already works like that, we can just take the behavior from them, if not, then we need to think🤔

[ntBre]

I think it's a bit weird to suggest diagnostic when the code can't run due to an error

I can see what you mean, but in general I think we try to show as many diagnostics as possible. The case I usually picture is someone getting a different diagnostic about the wrong number of arguments (maybe from a type checker or LSP), fixing that, and then a new Ruff diagnostic appears. Instead we'd prefer for the Ruff diagnostic to be there from the start. They are using (or trying to use) an os method replaceable by pathlib too, so I think the diagnostic is still accurate. I momentarily forgot this rule of thumb in my initial comment 😅

In this case it also preserves the existing behavior, which is nice since adding an autofix usually shouldn't modify the other behavior of the rule.

in some rules it already works like that, we can just take the behavior from them, if not, then we need to think🤔

Which rules do you mean here?

[chirizxc]

I think it's a bit weird to suggest diagnostic when the code can't run due to an error

I can see what you mean, but in general I think we try to show as many diagnostics as possible. The case I usually picture is someone getting a different diagnostic about the wrong number of arguments (maybe from a type checker or LSP), fixing that, and then a new Ruff diagnostic appears. Instead we'd prefer for the Ruff diagnostic to be there from the start. They are using (or trying to use) an os method replaceable by pathlib too, so I think the diagnostic is still accurate. I momentarily forgot this rule of thumb in my initial comment 😅

In this case it also preserves the existing behavior, which is nice since adding an autofix usually shouldn't modify the other behavior of the rule.

in some rules it already works like that, we can just take the behavior from them, if not, then we need to think🤔

Which rules do you mean here?

Misspelled, meant IF this behavior is already used in other rules, we should do the same thing

[chirizxc]

done

[ntBre]

Thank you!