Move impl From for Py<T> to macros not used in rust-numpy #1228
Conversation
There was a problem hiding this comment.
Thanks for debugging this. I 100% agree we should add a fix for this impl From to 0.12.2, as it is broken for anyone using these macros on Rust 1.39.
I also remove $(,$type_param: ident)* from macros not used in rust-numpy.
I can understand why you chose this cleanup. However I think that these macros should all follow the same form without special-casing for rust-numpy. So if we need $type_param for some, I think we should have $type_param for all of them. (See my other longer comment.)
src/types/mod.rs
Outdated
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<&'_ $name> for $crate::Py<$name> { |
There was a problem hiding this comment.
I think it would be nice for rust-numpy to have this impl once it is possible, so maybe can we add a FIXME to move this back to pyobject_native_type_named once we bump MSRV to Rust 1.40 or higher?
src/types/mod.rs
Outdated
| @@ -27,6 +27,7 @@ pub use self::string::{PyString, PyString as PyUnicode}; | |||
| pub use self::tuple::PyTuple; | |||
| pub use self::typeobject::PyType; | |||
|
|
|||
| // NOTE: this is used by rust-numpy and takes `$type_param`. | |||
There was a problem hiding this comment.
I must confess I am cautious of giving rust-numpy special treatment in the PyO3 codebase. We should definitely always make things possible for rust-numpy to work but I don't think we should be special-casing PyO3 code for rust-numpy. What if someone wants to work on e.g. rust-tensorflow using PyO3 and needs us to make further special cases? Also what if the macros used by rust-numpy change in future - we'd then need to make another PyO3 release to change these macros.
I think it's best if we make sure that PyO3 APIs are flexible enough to support all crates without needing to introduce special rules for a crate to work.
So I think really all of these macros should continue to take $type_param for best consistency.
kngwyu
force-pushed
the
exclude-py-coversion
branch
from
ddb1a4d to
f45242f
Compare
|
Thank you for your review! |
|
Thanks for addressing. I'll merge this now, then rebase + release 0.12.2 tonight.
Yeah, I think you're correct. Let's move it there in the future 👍 |
Implementing this for
PyArrayis not allowed in older Rust.Fixes PyO3/rust-numpy#159.
I also remove
$(,$type_param: ident)*from macros not used in rust-numpy.