Implement get/set all on pyclass #2692
Conversation
|
LGTM, I think |
pyo3-macros-backend/src/pyclass.rs
Outdated
| for (_, FieldPyO3Options { get, .. }) in &mut field_options { | ||
| if mem::replace(get, true) { | ||
| return Err(syn::Error::new(attr.span(), DUPE_GET)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if let Some(attr) = args.options.set_all { | ||
| for (_, FieldPyO3Options { set, .. }) in &mut field_options { | ||
| if mem::replace(set, true) { | ||
| return Err(syn::Error::new(attr.span(), DUPE_SET)); | ||
| } | ||
| } |
There was a problem hiding this comment.
IMO the spans for the errors here aren't ideal because they use the span of get_all and set_all, but the error is describing the get and set as the source of the error.
Using the syn::Field span (bound as _ in these snippets) may be better as it'll point to the field which has the invalid attribute?
The ideal span would probably be the get attribute itself - you could convert the FieldPyO3Options to have get: Option<kw::get>, and then use those spans. It would however mean that your elegant solution of using mem::replace here wouldn't be quite enough.
pyo3-macros-backend/src/pyclass.rs
Outdated
| const DUPE_SET: &str = "duplicate `set` - the struct is already annotated with `set_all`"; | ||
| const DUPE_GET: &str = "duplicate `get` - the struct is already annotated with `get_all`"; |
There was a problem hiding this comment.
Nit: I think I'd use "useless" or "redundant" rather than duplicate?
| const DUPE_SET: &str = "duplicate `set` - the struct is already annotated with `set_all`"; | |
| const DUPE_GET: &str = "duplicate `get` - the struct is already annotated with `get_all`"; | |
| const DUPE_SET: &str = "useless `set` - the struct is already annotated with `set_all`"; | |
| const DUPE_GET: &str = "useless `get` - the struct is already annotated with `get_all`"; |
|
Thank you for implementing this! Any idea when it'll make it to crates.io? Placing PyO3 attributes behind features is pretty essential for something I'm working on so I'm eager to use this. |
Fixes #1003 and most of #780
I'm happy to bikeshed the naming; I'm not sure whether I prefer
get_allor justget.