deprecate ToPyObject in favor or IntoPyObject
#4595
Conversation
Icxolu
force-pushed
the
deprecate/to-pyobject
branch
from
0374af6
to
5d4c362
Compare
There was a problem hiding this comment.
Thanks, this looks reasonable to me. I was wondering about punting on this deprecation until 0.24, however I also think given it's just a deprecation there's no harm in kicking it off sooner.
A bunch of thoughts related to documentation, as well as celebrating some nice cleanups! 🎉
guide/src/conversions/traits.md
Outdated
There was a problem hiding this comment.
We should add IntoPyObject to this file and update the section about ToPyObject to note that it is deprecated. Can leave it with an #[allow(deprecated)] example so that we remember to remove the section along with the trait in future.
There was a problem hiding this comment.
I have written docs as part of #4495. I could copy the relevant section into here if we want to merge them sooner.
| //! | ||
| //! fn main() { | ||
| //! fn main() -> PyResult<()> { |
There was a problem hiding this comment.
Nice touch to make these examples fallible and remove unwraps 👍
| // Also check that trying to convert an out of bound value errors. | ||
| Python::with_gil(|py| { | ||
| assert!(panic::catch_unwind(|| Duration::min_value().to_object(py)).is_err()); | ||
| assert!(panic::catch_unwind(|| Duration::max_value().to_object(py)).is_err()); | ||
| assert!(Duration::min_value().into_pyobject(py).is_err()); | ||
| assert!(Duration::max_value().into_pyobject(py).is_err()); |
There was a problem hiding this comment.
Yeah, it's really nice to see these finally go 🥳
| @@ -194,8 +198,7 @@ mod tests { | |||
| let mut map = hashbrown::HashMap::<i32, i32>::new(); | |||
| map.insert(1, 1); | |||
|
|
|||
| let m = map.to_object(py); | |||
| let py_map = m.downcast_bound::<PyDict>(py).unwrap(); | |||
| let py_map = (&map).into_pyobject(py).unwrap(); | |||
There was a problem hiding this comment.
❤️ it's nice to see the downcasts removed, even if there's suddenly a new fallible case!
There was a problem hiding this comment.
Yes! And getting a Bound directly instead of a Py is also super nice
Icxolu
force-pushed
the
deprecate/to-pyobject
branch
from
af09744
to
b04ca9e
Compare
Icxolu
force-pushed
the
deprecate/to-pyobject
branch
from
b04ca9e
to
a9fb13c
Compare
Icxolu
force-pushed
the
deprecate/to-pyobject
branch
from
a9fb13c
to
d81d00f
Compare
This deprecates
ToPyObjectin favor orIntoPyObject. All theimpls are[allow(deprecated)], tests were adapted to useIntoPyObject. This will probably cause some hit in coverage, but it should hopefully recover once we can remove a lot of this after 0.23.Since the diff went quite big, let me know if I should try to split this.