port Python::import to Bound API
#3832
Conversation
Icxolu
force-pushed
the
python-import
branch
from
db87a73 to
9c72a44
Compare
CodSpeed Performance ReportMerging #3832 will degrade performances by 28.99%Comparing Summary
Benchmarks breakdown
|
Icxolu
force-pushed
the
python-import
branch
from
9c72a44 to
02e4692
Compare
There was a problem hiding this comment.
Thanks yet again! Looks great, it's nice to see a whole bunch of .as_borrowed() and .as_gil_ref() calls getting removed.
Only one tiny adjustment which I thought could be worth doing, plus a couple of thoughts which aren't really for this PR and more follow-ups...
| @@ -1196,15 +1196,15 @@ impl<T> Py<T> { | |||
| /// # Example: `intern!`ing the attribute name | |||
| /// | |||
| /// ``` | |||
| /// # use pyo3::{intern, pyfunction, types::PyModule, IntoPy, Py, Python, PyObject, PyResult}; | |||
| /// # use pyo3::{prelude::*, intern}; | |||
| .to_cow()? | ||
| .into_owned(); |
There was a problem hiding this comment.
👍 nicely done figuring out the to_str / to_cow tweaks on PyString. I guess we should probably add a note about PyString::to_str to the migration guide; I don't think we have one at the moment...
There was a problem hiding this comment.
Yeah, thats a good idea. I think I stumbled over this twice already...
tests/test_datetime_import.rs
Outdated
| @@ -1,6 +1,6 @@ | |||
| #![cfg(not(Py_LIMITED_API))] | |||
|
|
|||
| use pyo3::{types::PyDate, Python}; | |||
| use pyo3::{prelude::PyAnyMethods, types::PyDate, Python}; | |||
There was a problem hiding this comment.
| use pyo3::{prelude::PyAnyMethods, types::PyDate, Python}; | |
| use pyo3::{prelude::*, types::PyDate}; |
Slightly related thought: I've been thinking we might want to make these traits public at some path other than the prelude, just for the few occasions when users might want to directly name them (?).
One option would be to make their existing paths public, e.g. pyo3::types::any::PyAnyMethods. We'd just need to be careful we didn't accidentally make other APIs public at the same time.
There was a problem hiding this comment.
We could also reexport them from pyo3::types, like we already do for the types themselfs
There was a problem hiding this comment.
pyo3::types I think makes a lot of sense. That module is already public, and we can control easily what gets exposed 👍
There was a problem hiding this comment.
I decided to commit the tiny fixup & merge now, and we can continue to discuss the follow-ups while potentially I rebase and get #3775 ready for review.
Part of #3684
This ports the
Python::importover to theBoundAPI. This does not portPyModule::import, which can be done separately and is currently part of #3775. I left a FIXME there, indicating that it needs to change in the future, but it should also popup oncePyModule::importgets deprecated.