[`flake8-use-pathlib`] Recommend `Path.iterdir()` over `os.listdir()` (`PTH208`) by InSyncWithFoo · Pull Request #14509 · astral-sh/ruff

InSyncWithFoo · 2024-11-21T10:32:32Z

Summary

Resolves #14490.

Test Plan

cargo nextest run and cargo insta test.

github-actions · 2024-11-21T10:43:39Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

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

+ dev/breeze/src/airflow_breeze/commands/release_management_commands.py:1134:17: PTH208 Use `pathlib.Path.iterdir()` instead.
+ dev/breeze/src/airflow_breeze/commands/release_management_commands.py:2181:29: PTH208 Use `pathlib.Path.iterdir()` instead.
+ dev/breeze/src/airflow_breeze/commands/sbom_commands.py:299:33: PTH208 Use `pathlib.Path.iterdir()` instead.
+ dev/breeze/src/airflow_breeze/utils/cdxgen.py:142:22: PTH208 Use `pathlib.Path.iterdir()` instead.
+ dev/check_files.py:204:13: PTH208 Use `pathlib.Path.iterdir()` instead.
+ dev/check_files.py:217:13: PTH208 Use `pathlib.Path.iterdir()` instead.
+ dev/check_files.py:230:13: PTH208 Use `pathlib.Path.iterdir()` instead.
+ docs/build_docs.py:457:20: PTH208 Use `pathlib.Path.iterdir()` instead.
+ providers/src/airflow/providers/amazon/aws/hooks/sagemaker.py:177:63: PTH208 Use `pathlib.Path.iterdir()` instead.
+ providers/tests/openlineage/plugins/test_execution.py:60:81: PTH208 Use `pathlib.Path.iterdir()` instead.
+ providers/tests/sftp/hooks/test_sftp.py:177:39: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/ci/pre_commit/version_heads_map.py:47:17: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tests_common/test_utils/system_tests_class.py:103:17: PTH208 Use `pathlib.Path.iterdir()` instead.
... 1 additional changes omitted for project

bokeh/bokeh (+11 -0 violations, +0 -0 fixes)

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

+ examples/server/app/simple_hdf5/main.py:19:28: PTH208 Use `pathlib.Path.iterdir()` instead.
+ src/bokeh/command/subcommands/__init__.py:54:17: PTH208 Use `pathlib.Path.iterdir()` instead.
+ src/bokeh/sphinxext/bokeh_gallery.py:134:18: PTH208 Use `pathlib.Path.iterdir()` instead.
+ src/bokeh/sphinxext/bokeh_gallery.py:160:21: PTH208 Use `pathlib.Path.iterdir()` instead.
+ src/bokeh/sphinxext/bokeh_releases.py:76:47: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tests/support/util/examples.py:186:33: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tests/unit/bokeh/command/subcommands/test___init___subcommands.py:48:13: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tests/unit/bokeh/command/subcommands/test_json__subcommands.py:111:54: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tests/unit/bokeh/command/subcommands/test_json__subcommands.py:123:50: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tests/unit/bokeh/command/subcommands/test_json__subcommands.py:135:50: PTH208 Use `pathlib.Path.iterdir()` instead.
... 1 additional changes omitted for project

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

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

+ corporate/tests/test_stripe.py:137:18: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/lib/run_hooks.py:63:38: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/lib/setup_venv.py:165:20: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/lib/zulip_tools.py:143:17: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/lib/zulip_tools.py:308:21: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/lib/zulip_tools.py:351:27: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/lib/zulip_tools.py:370:64: PTH208 Use `pathlib.Path.iterdir()` instead.
+ scripts/lib/zulip_tools.py:657:26: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tools/documentation_crawler/documentation_crawler/spiders/check_help_documentation.py:34:29: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tools/lib/test_script.py:102:34: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tools/setup/generate_landing_page_images.py:29:21: PTH208 Use `pathlib.Path.iterdir()` instead.
+ tools/setup/generate_zulip_bots_static_files.py:48:20: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/data_import/mattermost.py:879:8: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/data_import/slack.py:1431:8: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/data_import/slack.py:818:22: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/lib/sounds.py:10:22: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/management/commands/compilemessages.py:98:23: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/management/commands/convert_mattermost_data.py:61:12: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/management/commands/convert_rocketchat_data.py:39:12: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/management/commands/export.py:138:20: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/management/commands/export_single_user.py:41:47: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/tests/test_delete_unclaimed_attachments.py:75:17: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/tests/test_delete_unclaimed_attachments.py:94:17: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/tests/test_import_export.py:268:30: PTH208 Use `pathlib.Path.iterdir()` instead.
+ zerver/tests/test_urls.py:37:20: PTH208 Use `pathlib.Path.iterdir()` instead.
... 2 additional changes omitted for project

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PTH208	52	52	0	0	0

MichaReiser · 2024-11-22T15:16:28Z

I noticed two common patterns when reviewing the ecosystem checks:

if os.listdir("dir"): ....

if "file" in os.listdir("dir"):

The first requires using len, list, or any because using the Path.iterdir directly always returns true.

The second mainly becomes more verbose. I'm interested in more opinions if we should exclude them. Wdyt @sbrugman

https://github.com/apache/airflow/blob/440c224af5592f9007eef43d1dbe9025aa34e177/docs/build_docs.py#L456

https://github.com/bokeh/bokeh/blob/829b2a75c402d0d0abd7e37ff201fbdfd949d857/examples/server/app/simple_hdf5/main.py#L19
https://github.com/zulip/zulip/blob/65f05794ee59d638ad054ae6602d8ebc980fb637/scripts/lib/zulip_tools.py#L657
https://github.com/zulip/zulip/blob/65f05794ee59d638ad054ae6602d8ebc980fb637/zerver/data_import/mattermost.py#L879

InSyncWithFoo · 2024-11-22T15:28:03Z

The first requires using len, list, or any

To be pedantic, len() can't be used on an iterator, so only the other two should be suggested in that case.

sbrugman · 2024-11-25T10:15:54Z

The pathlib rules should flag all os.path cases. When these rules are active I assume users made the decision to favour pathlib over os.path, and partially excluding some examples will be unexpected.

It could be good to already include these cases in the tests to make sure they are covered when autofix is implemented later. The complexity here is in the fix, not in the detection of the violation.

Going over the ecosystem results I realise that os.scandir should also be flagged (and is closer to Path.iterdir as it produces a generator). @InSyncWithFoo it's probably worth adding this as PTH209. The non-trivial fixes stem from unidiomatic use of os.path in the first place imo.

Using listdir for checking that a file does not exist is even a candidate for it's own rule as this first lists all files in a directory, and then only checks one:

'demo_data.hdf5' not in os.listdir(app_dir)

Idiomatic os.path solution:

not os.path.exists(os.path.join(app_dir), 'demo_data.hdf5')

Pathlib equivalent:

not (app_dir / "demo_data.hdf5").exists()

Checking if a directory is empty with os.listdir is also wasteful:

not os.listdir(api_dir)

Users should probably write something like:

next(os.scandir(api_dir), None) is None

Pathlib equivalent:

next(api_dir.iterdir(), None) is None

InSyncWithFoo · 2024-11-25T11:11:10Z

it's probably worth adding this as PTH209.

Got it. I'll get to that the day after tomorrow.

InSyncWithFoo · 2024-11-25T14:30:51Z

@sbrugman On second thought... should PTH208 also check for os.scandir? These two seem sufficiently similar in both functionality and rule implementation.

sbrugman · 2024-11-25T15:26:27Z

No, adding a separate rule is consistent with other rules:

https://docs.astral.sh/ruff/rules/os-remove/
https://docs.astral.sh/ruff/rules/os-unlink/

The distinction could be useful for users who would like to fix os.scandir (since its replacement is also a generator and requires little effort), but not os.listdir for instance.

MichaReiser

Thanks @sbrugman. That makes sense to me. @InSyncWithFoo happy to merge this rule once we added tests demonstrating the patterns linked in #14509 (comment)

InSyncWithFoo · 2024-11-27T09:27:02Z

@MichaReiser Done. I added them to the rule's documentation as well.

crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs

… (`PTH208`)

MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Nov 22, 2024

InSyncWithFoo mentioned this pull request Nov 27, 2024

[flake8-use-pathlib] Recommend Path.iterdir() over os.scandir() (PTH209) #14623

Closed

MichaReiser requested changes Nov 27, 2024

View reviewed changes

MichaReiser reviewed Nov 27, 2024

View reviewed changes

crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs Show resolved Hide resolved

InSyncWithFoo and others added 5 commits November 27, 2024 10:45

[flake8-use-pathlib] Recommend Path.iterdir() over os.listdir()…

92e6957

… (`PTH208`)

Schema

6772669

Extraneous closing parenthesis

972c3a7

Per review

b3dc296

Remove preview-only test group

01f5b7f

MichaReiser force-pushed the PTH208 branch from cf69aa1 to 01f5b7f Compare November 27, 2024 09:48

MichaReiser approved these changes Nov 27, 2024

View reviewed changes

MichaReiser enabled auto-merge (squash) November 27, 2024 09:50

MichaReiser merged commit 187974e into astral-sh:main Nov 27, 2024

InSyncWithFoo deleted the PTH208 branch November 27, 2024 09:57

BrewTestBot mentioned this pull request Nov 29, 2024

ruff 0.8.1 Homebrew/homebrew-core#199449

Merged

MichaReiser mentioned this pull request Dec 2, 2024

Revert: [pyflakes] Avoid false positives in @no_type_check contexts (F821, F722) (#14615) #14726

Merged

InSyncWithFoo mentioned this pull request May 7, 2025

[flake8-use-pathlib] Recommend Path.iterdir() over os.scandir() (PTH209) #17906

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`flake8-use-pathlib`] Recommend `Path.iterdir()` over `os.listdir()` (`PTH208`)#14509

[`flake8-use-pathlib`] Recommend `Path.iterdir()` over `os.listdir()` (`PTH208`)#14509
MichaReiser merged 5 commits intoastral-sh:mainfrom
InSyncWithFoo:PTH208

InSyncWithFoo commented Nov 21, 2024

Uh oh!

github-actions bot commented Nov 21, 2024 •

edited

Loading

Uh oh!

MichaReiser commented Nov 22, 2024

Uh oh!

InSyncWithFoo commented Nov 22, 2024

Uh oh!

sbrugman commented Nov 25, 2024 •

edited

Loading

Uh oh!

InSyncWithFoo commented Nov 25, 2024

Uh oh!

InSyncWithFoo commented Nov 25, 2024 •

edited

Loading

Uh oh!

sbrugman commented Nov 25, 2024

Uh oh!

MichaReiser left a comment

Uh oh!

InSyncWithFoo commented Nov 27, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

InSyncWithFoo commented Nov 21, 2024

Summary

Test Plan

Uh oh!

github-actions bot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser commented Nov 22, 2024

Uh oh!

InSyncWithFoo commented Nov 22, 2024

Uh oh!

sbrugman commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

InSyncWithFoo commented Nov 25, 2024

Uh oh!

InSyncWithFoo commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbrugman commented Nov 25, 2024

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

InSyncWithFoo commented Nov 27, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Nov 21, 2024 •

edited

Loading

`ruff-ecosystem` results

sbrugman commented Nov 25, 2024 •

edited

Loading

InSyncWithFoo commented Nov 25, 2024 •

edited

Loading