-
Notifications
You must be signed in to change notification settings - Fork 911
Implement auto_new attribute for #[pyclass]
#5421
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
base: main
Are you sure you want to change the base?
Changes from all commits
5d9eaa1
a0edb21
0ed977c
ab2ca31
b6bcb50
3f675ee
036004f
9f652c0
10175ac
eddcad5
187d8e5
4881636
c2dfd2e
05eb42c
efea232
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Implement `new = "from_fields"` attribute for `#[pyclass]` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,6 +235,42 @@ fn test_renaming_all_struct_fields() { | |
| }); | ||
| } | ||
|
|
||
| #[pyclass(get_all, set_all, new = "from_fields")] | ||
| struct AutoNewCls { | ||
| a: i32, | ||
| b: String, | ||
| c: Option<f64>, | ||
| } | ||
RedKinda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| #[test] | ||
| fn new_impl() { | ||
| Python::attach(|py| { | ||
| // python should be able to do AutoNewCls(1, "two", 3.0) | ||
| let cls = py.get_type::<AutoNewCls>(); | ||
| pyo3::py_run!( | ||
| py, | ||
| cls, | ||
| "inst = cls(1, 'two', 3.0); assert inst.a == 1; assert inst.b == 'two'; assert inst.c == 3.0" | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to add a test with a tuple struct, e.g. struct Point2d(f64, f64);... I would think the generated constructor would allow only positional inputs, as there are no names for the fields.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (It might be good enough for this to gracefully fail with a "not yet supported" message as far as this PR is concerned.)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unless i am missing something i added support for tuple structs here 4881636 |
||
| #[pyclass(new = "from_fields", get_all)] | ||
| struct Point2d(#[pyo3(name = "first")] f64, #[pyo3(name = "second")] f64); | ||
|
|
||
| #[test] | ||
| fn new_impl_tuple_struct() { | ||
| Python::attach(|py| { | ||
| // python should be able to do AutoNewCls(1, "two", 3.0) | ||
| let cls = py.get_type::<Point2d>(); | ||
| pyo3::py_run!( | ||
| py, | ||
| cls, | ||
| "inst = cls(0.2, 0.3); assert inst.first == 0.2; assert inst.second == 0.3" | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| macro_rules! test_case { | ||
| ($struct_name: ident, $rule: literal, $field_name: ident, $renamed_field_name: literal, $test_name: ident) => { | ||
| #[pyclass(get_all, set_all, rename_all = $rule)] | ||
|
|
||
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.
We should have a quick think about the interaction of
#[pyo3(new = "default")]and the other future extensions I suggested in #5421 (review)new = "default"accept any arguments? The easy answer is no. But what if users want to have a constructor which is the equivalent ofMyStruct { x, y, ...Default::default() }, i.e. use the struct-level default except for some specific fields?#[pyo3(new = <value>)]to remove them as arguments from the generated constructor and always set them to<value>(similar todataclasses.field(init = False))? This would appear to have very strong overlap with #[pyo3(new = "default")]on the struct, presumablyfor the field would come from theDefault` implementation."from_fields"with a bunch of#[pyo3(default = <value>)]annotations on fields?... it feels to me like the general design would be that
new = "from_fields"would not require aDefaultimplementation, and would allow users to take fields out of the constructor and instead give them default values via#[pyo3(default = <value>)].new = "default"would be the opposite; it would require aDefaultimplementation and would require users to opt-in to add fields to the constructor to allow callers to override the value set by theDefaultimplementation. It feels like#[pyo3(new = true)]might be the right annotation for this, but I can't decide. That can in theory be a future extension fornew = "default"so maybe it's a long way off.We don't need to solve this now, but I'd at least like to make sure that having
"from_fields"as implemented here doesn't accidentally close off the design space. We can write this all into a follow-up issue after merge.Uh oh!
There was an error while loading. Please reload this page.
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.
If using
new = "from_fields", i think field-level defaults should make that field a keyword argument in__new__and give it the configured default. Example:would be the equivalent of
Using
new = "default"could then simply always produce an empty__new__.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.
What if a user didn't want
bto be user-providable at all? Maybe a#[pyo3(new_arg = false)]on the field e.g. maybe this:would produce Python API
and the Rust implementation would be equivalent to
Seems reasonable, and users could add arguments with
#[pyo3(new_arg = true)]? e.g.would produce Python API
and a Rust implemementation equivalent to