-
Notifications
You must be signed in to change notification settings - Fork 887
Add file #5205
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
base: main
Are you sure you want to change the base?
Add file #5205
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, and sorry for the slow review.
While I'm aware that IO type hierarchy is not mapped out by PyO3 at all, I have a number of concerns about the way this PR is designed which leave me unconvinced it's the way to move forward.
Without this PR, users can already do things like PyModule::import("io")
to interact with the IO types at runtime, and anything we'd do in PyO3 would just be a convenience layer which centralises maintenance burden. I'm not sure we have a solid enough proposal to consider taking that on yet.
If there is a particular use case you were trying to solve, perhaps start from there, and we can help you that way?
#[repr(transparent)] | ||
pub struct PyFileObject(PyObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This typedef doesn't exist in https://github.com/python/cpython/blob/main/Include/cpython/fileobject.h, we should not invent this.
PyFile, | ||
ffi::PyFileObject, | ||
pyobject_native_static_type_object!(ffi::PyBaseObject_Type), | ||
#checkfunction=PyObject_Check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow any object to be cast to PyFile
, which is definitely wrong. I don't think there's a good way to typecheck file
objects.
/// Represents a Python `file` object. | ||
/// | ||
/// Values of this type are accessed via PyO3's smart pointers, e.g. as | ||
/// [`Py<PyFile>`][crate::Py] or [`Bound<'py, PyFile>`][Bound]. | ||
#[repr(transparent)] | ||
pub struct PyFile(PyAny); | ||
|
||
pyobject_native_type!( | ||
PyFile, | ||
ffi::PyFileObject, | ||
pyobject_native_static_type_object!(ffi::PyBaseObject_Type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no such thing as a file
object in Python 3:
>>> f = open('foo.txt', 'w')
>>> f
<_io.TextIOWrapper name='foo.txt' mode='w' encoding='UTF-8'>
... we can't expose this as a native type like this. There might be something we can do with the io
module, however that module doesn't have any C API to interact with it and we are typically very careful about adding support for anything which doesn't have a C API.
/// For python file, do nothing in rust | ||
pub name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs for https://docs.python.org/3/c-api/file.html#c.PyFile_FromFd, name
is ignored on all our supported Python versions (3.2+).
/// For python file, do nothing in rust | ||
/// | ||
/// See | ||
/// <https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs updating.
I created a file-like type for PyO3. Since file is a fundamental type in Python, I believe it should be supported by this library.
Example:
I created a pyo3 type because the Python file uses three parameters that are not directly available in Rust::
I opened this pull request to check if this approach is acceptable.