PyO3 / pyo3

Rust bindings for the Python interpreter
https://pyo3.rs
Other
11.47k stars 694 forks source link

Added `From<Bound<'py, T>>` impl for `PyClassInitializer<T>`. #4214

Closed JRRudy1 closed 1 month ago

JRRudy1 commented 1 month ago

This is a tiny change that allows Bound<T> to be returned from a pyclass's #[new] method, as is already supported for Py<T>.

This flexibility would be convenient in certain cases; for example, it is often desirable to have a constructor for use on the Rust-side that returns the type as a Python object (Py<T> or Bound<T>) instead of T directly (e.g. when you want to be able to downcast into T::BaseClass). And as long your desired arguments are the same as for your #[new] method, you can just reuse the same constructor for use from both Python and Rust. However, you are currently limited to getting a Py<T> from this, which usually has to be followed by .into_bound(py) if you want to do anything useful with it. But if you could have the #[new] method return Bound<T> instead, as implemented by this PR, then it would be more ergonomic to call from Rust (while making no difference on the Python side). I also think it just makes sense intuitively, since Bound<T> is essentially just Py<T> with superpowers.

In my opinion this change is too trivial to warrant adding tests, but let me know if you disagree and I can add some.

alex commented 1 month ago

Please add a test -- they're not just to ensure that this implementation is correct (and I agree, it looks trivial), they're also to ensure that as we refactor and change our APIs in the future, we don't break this.

JRRudy1 commented 1 month ago

Please add a test -- they're not just to ensure that this implementation is correct (and I agree, it looks trivial), they're also to ensure that as we refactor and change our APIs in the future, we don't break this.

No problem, done!

JRRudy1 commented 1 month ago

Actually something failed, sorry I'll fix it.

alex commented 1 month ago

Thanks

JRRudy1 commented 1 month ago

No problem, thanks for the merge!