Skip to content

Makes PathBuf FromPyObject implementation work on all os.PathLike #3374

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.

Sign up for GitHub

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

Merged
adamreichold merged 2 commits into from
Aug 9, 2023
Merged

Makes PathBuf FromPyObject implementation work on all os.PathLike #3374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e5a495
Makes PathBuf FromPyObject implementation work on all os.PathLike
Tpt Aug 8, 2023
dd04d2c
Fixes linking of `PyOS_FSPath` on PyPy.
Tpt Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions guide/src/conversions/tables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ The table below contains the Python type and the corresponding function argument
| `decimal.Decimal` | `rust_decimal::Decimal`[^5] | - |
| `ipaddress.IPv4Address` | `std::net::IpAddr`, `std::net::IpV4Addr` | - |
| `ipaddress.IPv6Address` | `std::net::IpAddr`, `std::net::IpV6Addr` | - |
| `os.PathLike ` | `PathBuf`, `Path` | `&PyString`, `&PyUnicode` |
| `pathlib.Path` | `PathBuf`, `Path` | `&PyString`, `&PyUnicode` |
Copy link
Copy Markdown
Member

@adamreichold adamreichold Aug 8, 2023

Choose a reason for hiding this comment

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

Isn't pathlib.Path also an os.PathLike, i.e. doesn't the first line basically include the second one?

Copy link
Copy Markdown
Contributor Author

@Tpt Tpt Aug 8, 2023

Choose a reason for hiding this comment

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

Indeed. I left the two lines for explictness because I assumed a lot of people are not used to os.PathLike. If you prefer I can remove the pathlib.Path line.

Copy link
Copy Markdown
Member

@adamreichold adamreichold Aug 8, 2023

Choose a reason for hiding this comment

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

Personally, I would merge them and maybe use "pathlib.Path, os.PathLike" for the first column to stay explicit. But let's see what the other maintainers think.

Copy link
Copy Markdown
Member

@davidhewitt davidhewitt Aug 9, 2023

Choose a reason for hiding this comment

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

I was thinking of upending this table anyway and describing it from the Rust types (what they accept and what they convert to), so I'm ok with leaving this as two rows for now.

| `typing.Optional[T]` | `Option<T>` | - |
| `typing.Sequence[T]` | `Vec<T>` | `&PySequence` |
Expand Down
1 change: 1 addition & 0 deletions newsfragments/3374.added.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
`PathBuf` `FromPyObject` implementation now works on all `os.PathLike` values.
1 change: 1 addition & 0 deletions newsfragments/3374.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Linking of `PyOS_FSPath` on PyPy.
1 change: 1 addition & 0 deletions pyo3-ffi/src/osmodule.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::object::PyObject;

extern "C" {
#[cfg_attr(PyPy, link_name = "PyPyOS_FSPath")]
pub fn PyOS_FSPath(path: *mut PyObject) -> *mut PyObject;
}
24 changes: 24 additions & 0 deletions pytests/tests/test_path.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import pathlib

import pytest

import pyo3_pytests.path as rpath


Expand All @@ -16,3 +18,25 @@ def test_take_pathbuf():
def test_take_pathlib():
p = pathlib.Path("/root")
assert rpath.take_pathbuf(p) == str(p)


def test_take_pathlike():
Comment thread
adamreichold marked this conversation as resolved.
assert rpath.take_pathbuf(PathLike("/root")) == "/root"


def test_take_invalid_pathlike():
with pytest.raises(TypeError):
assert rpath.take_pathbuf(PathLike(1))


def test_take_invalid():
with pytest.raises(TypeError):
assert rpath.take_pathbuf(3)


class PathLike:
def __init__(self, path):
self._path = path

def __fspath__(self):
return self._path
24 changes: 7 additions & 17 deletions src/conversions/std/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use crate::intern;
use crate::{FromPyObject, IntoPy, PyAny, PyObject, PyResult, Python, ToPyObject};
use crate::{
ffi, AsPyPointer, FromPyObject, FromPyPointer, IntoPy, PyAny, PyObject, PyResult, Python,
ToPyObject,
};
use std::borrow::Cow;
use std::ffi::OsString;
use std::path::{Path, PathBuf};
Expand All @@ -14,21 +16,9 @@ impl ToPyObject for Path {

impl FromPyObject<'_> for PathBuf {
fn extract(ob: &PyAny) -> PyResult<Self> {
let os_str = match OsString::extract(ob) {
Ok(s) => s,
Err(err) => {
let py = ob.py();
let pathlib = py.import(intern!(py, "pathlib"))?;
let pathlib_path = pathlib.getattr(intern!(py, "Path"))?;
if ob.is_instance(pathlib_path)? {
let path_str = ob.call_method0(intern!(py, "__str__"))?;
OsString::extract(path_str)?
} else {
return Err(err);
}
}
};
Ok(PathBuf::from(os_str))
// We use os.fspath to get the underlying path as bytes or str
let path = unsafe { PyAny::from_owned_ptr_or_err(ob.py(), ffi::PyOS_FSPath(ob.as_ptr())) }?;
Ok(OsString::extract(path)?.into())
}
}

Expand Down