-
Notifications
You must be signed in to change notification settings - Fork 803
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
Implement ToPyObject
for Box<T>
#3729
Conversation
CodSpeed Performance ReportMerging #3729 will degrade performances by 11.41%Comparing Summary
Benchmarks breakdown
|
Thanks for the PR! There is some prior discussion in #3014 where we decided to pause on adding |
fn test_box_intopy() { | ||
let s: Box<str> = "test".into(); | ||
|
||
let obj: PyObject = Python::with_gil(|py| s.into_py(py)); |
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.
The fact that I need to specifically annotate the type of obj
here makes me suspicious that implementing IntoPy
like above is doing something funky.
Aha, thanks for the pointer to the PR. It seemed odd that this hadn't come up before, but failed to find any issues about it. My main use case for using
And then when trying to return such things to Python often requires suitable derefs etc. TBH this isn't the end of the world once the code has been written, its just a little bit of a pain to get it right when writing it (especially when converting from RE the discussion on the PR: I understand that This PR is really just making things slightly more ergonomic for a fairly niche use case, so I'm more than happy if this is something we want to hold back from doing right now. |
Thanks for the understanding. I think for now I will close this, but I've added a reminder in #1089 to come back to these implementations once that design question is solved. |
There are implementations for
&T
,Vec<T>
etc, so I think it makes sense to also do forBox<T>
. I'm also happy to add implementations forArc
etc?The motivation here is to be able to use
Box<str>
more easily (as its got slightly less memory overhead than String)I'm not sure if it makes sense to also implement
IntoPy
, as it's just doing the same asToPyObject
. Happy to remove it.Note also that adding I couldn't add an implementation for
FromPyObject
as I got a "conflicting implementation" error:I confess I don't understand why this is a problem for
Box<_>
and not forOption<_>
.