use extract_argument for #[setter] extraction#3998
Conversation
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks! Just a couple of suggestions for this one...
| } | ||
| }; | ||
|
|
||
| let holder = holders.push_holder(span); |
There was a problem hiding this comment.
Maybe add a TODO to rework this to use impl_arg_param later? I think the hardest part to doing so would be that impl_arg_param assumes the arguments are in an array and takes the array ident plus an index. We could probably rework that so that impl_arg_param just gets a TokenStream of something to extract from.
(Of course, if you're up for it, can also go for the refactoring now 👀)
There was a problem hiding this comment.
I added, the TODO for now, I think if we refactor this, this should also get rid of the from_py_with case, since that is handled in impl_arg_param already. I think it makes sense to do that, for consistency and code deduplication, but probably deserves it's own PR. (We would also need to figure out how to handle the Descriptor case)
There was a problem hiding this comment.
Agreed, it would be a fair bit of rewiring, hopefully with a rewarding payoff at the end! 👍
Icxolu
force-pushed
the
fix-setter-extraction
branch
from
690c5e0 to
1243232
Compare
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Looks good to me, thanks! 👍
2 participants
Following #3995 (comment)
This updates the
#[setter]argument toextract_argumentinstead ofFromPyObject::extract_bound. This allows extracting the same types as normal#[pyfunction]arguments, in particular&Bound. This also adds the deprecation warnings for gil-ref extraction.