Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert PathBuf & Path into python pathlib.Path instead of PyString #4925

Merged
merged 3 commits into from
Feb 26, 2025

Conversation

bschoenmaeckers
Copy link
Contributor

@bschoenmaeckers bschoenmaeckers commented Feb 19, 2025

closes #4907

@bschoenmaeckers bschoenmaeckers force-pushed the pathbuf-pathlib branch 3 times, most recently from 7a4cd73 to 5adfc6c Compare February 20, 2025 09:46
@LilyFoote
Copy link
Contributor

How do we handle paths that are not utf-8? I believe both Rust and Python have ways to represent these, but I think especially on the Rust side, this is considered an implementation detail and I'm not sure we can safely convert between them.

@Icxolu
Copy link
Contributor

Icxolu commented Feb 21, 2025

Not a full review (yet), but I think we need to keep the current behavior for the deprecated IntoPy and ToPyObject impls.

@bschoenmaeckers
Copy link
Contributor Author

How do we handle paths that are not utf-8? I believe both Rust and Python have ways to represent these, but I think especially on the Rust side, this is considered an implementation detail and I'm not sure we can safely convert between them.

We are using the &OsStr conversions defined in the ostr.rs file. I did not change that in this MR. Do you have an example of a non-utf8 string that may cause problems? So I can add it as a test case.

@LilyFoote
Copy link
Contributor

How do we handle paths that are not utf-8? I believe both Rust and Python have ways to represent these, but I think especially on the Rust side, this is considered an implementation detail and I'm not sure we can safely convert between them.

We are using the &OsStr conversions defined in the ostr.rs file. I did not change that in this MR. Do you have an example of a non-utf8 string that may cause problems? So I can add it as a test case.

Oh, I didn't know we had that code, that's really cool! I think my concern is moot.

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

A side from keeping old conversion for IntoPy and ToPyObject I only have a small suggestion around the tests.

Comment on lines 187 to 194
let pystring = unsafe {
ffi::PyOS_FSPath(pyobject.as_ptr())
.assume_owned_or_err(py)
.unwrap()
.downcast_into::<PyString>()
.unwrap()
};
assert_eq!(pystring.to_string_lossy(), obj.as_ref().to_string_lossy());
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether it still makes sense to compare these as strings, now that we don't have a string representation on either side. Maybe we can just get rid of this intermediate check.

Instead I think we should add a new test that a PathBuf can be extracted from both a str and pathlib.Path, which was removed from this test.

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much!

@Icxolu Icxolu added this pull request to the merge queue Feb 26, 2025
Merged via the queue into PyO3:main with commit ffd7276 Feb 26, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support converting std::path::{Path, PathBuf} to pathlib.Path
3 participants