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

Implement PyClass for PathBuf and Path #1377

Closed
kangalio opened this issue Jan 10, 2021 · 9 comments
Closed

Implement PyClass for PathBuf and Path #1377

kangalio opened this issue Jan 10, 2021 · 9 comments

Comments

@kangalio
Copy link
Contributor

There exists an infallible conversion from String to PathBuf (and hence Path), so if I'm not missing something, there is no real reason not to implement PyClass for PathBuf/Path.

That would allow Rust functions exposed to Python to take paths as parameters with the actual appropriate Rust type, instead of having to use String as a workaround.

@kangalio
Copy link
Contributor Author

I'm trying to make a PR for this

@davidhewitt
Copy link
Member

I think rather than PyClass, you should implement IntoPy<PyObject>, ToPyObject, and FromPyObject for PathBuf (and maybe Path, if you can make it work).

@kangalio
Copy link
Contributor Author

I think it will be better to implement those three traits on OsStr and OsString, because that's what the this work is really about: converting between Python's magic strings and Rust's opaque OS strings.

Path and PathBuf can just redirect to the implementations on OsStr and OsString.

@davidhewitt
Copy link
Member

That sounds sensible to me! If you're looking at string types, it could be worth assessing if CStr and CString should also be supported. I think Python allows null bytes in the middle of its strings so definitely from-Python would be fallible for those.

@kangalio
Copy link
Contributor Author

Should the conversion functions from/to OsStr/Cstr go onto the PyString struct? It would get four new methods:

  • from_osstr
  • to_osstr
  • from_cstr
  • to_cstr

The alternative would be to create new modules types/cstr.rs and types/osstr.rs, and implement the conversions there

@davidhewitt
Copy link
Member

I would suggest avoiding new methods on PyString if we can - as much as possible the methods on the python-native types are just wrappers around the underlying Python C API.

Is there a reason the traits above don't provide enough functionality for what you need? The traits are all we need for #[pyfunction] / #[pymethods] etc.

@kangalio
Copy link
Contributor Author

kangalio commented Jan 10, 2021

I was just asking what you preferred, I'm fine with both.

In any case, I wrote the trait implementations for OsStr/OsString: https://github.com/kangalioo/pyo3/blob/bbc578fda767134ff678c1ebc62eae5e0e756cb2/src/types/osstr.rs. I'd greatly appreciate if you could check it out to make sure I didn't do anything totally stupid. Comments with // HELP mark places where I was unsure if I was doing the right thing.

In the meantime, I'll be working on CStr/CString and Path/PathBuf.

EDIT: Actually, I'm not gonna do the conversions for CStr/CString in this PR, since they're not really strings but byte arrays, so they don't fit with the rest of the PR. (And also because I don't see a big need to transfer C strings between Rust and Python and the work to support it doesn't seem to be worth it for me right now)

@davidhewitt
Copy link
Member

👍 totally makes sense - nice work and thanks for the PR; I'll do my best to make time to review it as soon as I can.

@davidhewitt
Copy link
Member

Closed by #1379 . Thanks again for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants