-
Notifications
You must be signed in to change notification settings - Fork 763
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
Improvements to property macros #760
Conversation
The CI failures are from new clippy lints from the newer rust version. I've added a PR #761 to fix those separately. |
T: IntoPy<PyObject> + Clone, | ||
{ | ||
fn get_property_value(&self, py: Python) -> PyObject { | ||
(*self).clone().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.
Why Deref
here... ?
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.
&self
is &&T
, so self.clone()
makes &T
. We need to deref like (*self).clone()
to get a T
.
Thank you! |
|
||
impl<T> GetPropertyValue for &T | ||
where | ||
T: IntoPy<PyObject> + Clone, |
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.
I was wondering whether the Clone
bound should really be Copy
, so that we don't let people accidentally do really expensive properties with #[pyo3(get)]
(such as Vec<T>
).
Users can always write their own #[getter]
functions for types which aren't Copy
.
Thanks, I've made some changes to address your comments. I suggest we wait for #761 to merge first. After that I'll fix up the merge conflicts in this PR and then we can potentially merge this. |
Thank you! |
Thanks for fixing the merge conflicts! |
There are two main fixes that I've added to the property macro code:
#[pyo3(get)]
now works withPyObject
fields#[setter]
functions can now take aPython
argumentAfter implementing the first I'd changed enough of the code such that cleaning it up caused the second to be fixed trivially.
I also bumped the minimum Rust version to
1.42.0-nightly 2020-01-21
, to make use of the recently stabilized#![feature(slice_patterns)]
.