Skip to content

fix(PTH208): add display-only hint for os.listdir - #28027

Open
fly1d wants to merge 2 commits into
astral-sh:mainfrom
fly1d:codex/pth208-display-only
Open

fix(PTH208): add display-only hint for os.listdir#28027
fly1d wants to merge 2 commits into
astral-sh:mainfrom
fly1d:codex/pth208-display-only

Conversation

@fly1d

@fly1d fly1d commented Aug 25, 2026

Copy link
Copy Markdown

Summary

Adds a display-only suggestion for PTH208 diagnostics with a path argument:

Path(path).iterdir()

The suggestion is intentionally not an automatic fix because os.listdir returns strings or bytes, while Path.iterdir() returns Path objects and can change downstream behavior. The existing diagnostic remains unchanged, and file-descriptor calls do not receive the suggestion.

Part of #2331.

Verification

  • INSTA_FORCE_PASS=1 INSTA_UPDATE=always cargo test -p ruff_linter flake8_use_pathlib::tests --lib (34 passed)
  • cargo fmt --all -- --check
  • git diff --check

OpenAI Codex assisted with implementation and verification. The contributor reviewed the change before submission.

@astral-sh-bot
astral-sh-bot Bot requested a review from ntBre August 25, 2026 05:58
OpenAI Codex assisted with implementation and verification. The contributor reviewed the change before submission.
@fly1d
fly1d force-pushed the codex/pth208-display-only branch from 9882e6d to 39b40ac Compare August 25, 2026 05:59
@astral-sh-bot

astral-sh-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

We support using AI (i.e., LLMs) while coding. However, AI should not be used to communicate with maintainers.

Please read and follow our AI policy when submitting issues and pull requests.

@astral-sh-bot astral-sh-bot Bot closed this Aug 25, 2026
@fly1d

fly1d commented Aug 26, 2026

Copy link
Copy Markdown
Author

I confirm that I have read the AI policy. However, as a non-native English speaker, I may not have fully understood all the details. Please note that the comments were originally written by me, with AI used solely for language polishing.

@ntBre

ntBre commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Your test plan specifically read as very LLM-generated to me, but I'll take you at your word and reopen this. Thank you for reading the policy.

@ntBre ntBre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This looks reasonable at a high level, but we still need to check that the diagnostic is enabled and also use the importer infrastructure to ensure that Path is in scope for the fix. I also had a couple of smaller suggestions.

Comment on lines +95 to +96
let mut diagnostic = checker.report_diagnostic(OsListdir, range);
if let Some(path) = call.arguments.find_argument_value("path", 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need to use report_diagnostic_if_enabled here, or we could report the diagnostic when the rule is disabled. Something like this should work:

Suggested change
let mut diagnostic = checker.report_diagnostic(OsListdir, range);
if let Some(path) = call.arguments.find_argument_value("path", 0) {
if let Some(mut diagnostic) = checker.report_diagnostic_if_enabled(OsListdir, range) {
if let Some(path) = call.arguments.find_argument_value("path", 0) {

It also looks like we don't need to return the diagnostic here, I think the match arms are just missing semicolons so that we can drop the Some(diagnostic).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I switched to report_diagnostic_if_enabled, and this branch no longer returns Some(diagnostic).

Comment on lines +95 to +96
let mut diagnostic = checker.report_diagnostic(OsListdir, range);
if let Some(path) = call.arguments.find_argument_value("path", 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse the find_argument_value call from above? I'm picturing something like:

let path = call.arguments.find_argument_value("path", 0);

if path.is_some_and(|expr| is_file_descriptor(expr, checker.semantic())) {
    return;
}

then your code from above.

@fly1d fly1d Aug 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure,I now look up path only once and use it for both the file-descriptor check and the replacement.

let mut diagnostic = checker.report_diagnostic(OsListdir, range);
if let Some(path) = call.arguments.find_argument_value("path", 0) {
diagnostic.set_fix(Fix::display_only_edit(Edit::range_replacement(
format!("Path({}).iterdir()", checker.locator().slice(path)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure Path is actually in scope. See some other rules for examples:

let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import("pathlib", "Path"),
call.start(),
checker.semantic(),
)?;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright – we can also rely on the importer to reuse an existing Path binding, or add the import when needed.

pub(crate) struct OsListdir;

impl Violation for OsListdir {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a fix_title method now to customize the hint shown for the diagnostic.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a custom fix_title method per your suggestion.

@ntBre ntBre added the fixes Related to suggested fixes for violations label Aug 26, 2026
@ntBre

ntBre commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

As one other note, I think we've mosty added pathlib fixes in preview, but that's only required for safe fixes by our versioning policy. I think this display-only fix is fine to add in stable.

@fly1d
fly1d force-pushed the codex/pth208-display-only branch 2 times, most recently from 4f4b004 to 39b40ac Compare August 27, 2026 02:56
Use the diagnostic-enabled reporting path and the importer when suggesting a pathlib replacement for os.listdir. Add a dedicated fix title and update the snapshot.

OpenAI Codex assisted with implementation and verification. I reviewed the changes before submission.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fixes Related to suggested fixes for violations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants