refactor #[setter] argument extraction
#4002
Conversation
There was a problem hiding this comment.
Nice, thanks! I think even if this is a net increase in code size right now, I'm optimistic that it'll get much nicer once the migration is complete, as you say.
Is there a particular reason why you kept this in draft? Otherwise I would be happy so see this merged already and then further refactoring can be done later.
| pub fn is_regular(&self) -> bool { | ||
| !self.py && !self.is_cancel_handle && !self.is_kwargs && !self.is_varargs | ||
| } |
There was a problem hiding this comment.
Seeing this I'm thinking FnArg might want to be reworked to be an enum. Not sure if it's worth trying to make that part of this PR or we punt for another time.
| if arg.is_regular() { | ||
| option_pos += 1; | ||
| } |
There was a problem hiding this comment.
This is a little unfortunate to have the coupling between incrementing the option and impl_arg_param. I guess that having the enum, then impl_arg_param could be used just for "Regular" variants and then other args like py or cancel_handle could be their own branches.
Maybe it's better for me to try to implement what I mean than add a drive by comment for that, though?
There was a problem hiding this comment.
Feel free to continue/add something here if you have something in mind. I wasn't really satisfied by how this turned out either (which is why I left it as draft for now)
There was a problem hiding this comment.
Sure thing, I'll try to find a good moment to do this soon
There was a problem hiding this comment.
I have played around a bit with your enum idea over here. Maybe that goes into the direction that you hand in mind? That does definetly decouple the different branches from each other, but it's a bit more involved and not really relevant to the #[setter] rework here. So maybe you are right to leave this as is, and rework FnArg in a followup.
There was a problem hiding this comment.
That definitely looks like the idea I had in mind. Does it feel like a step in the right direction as you're working on it?
There was a problem hiding this comment.
It does feel nicer in my opinion. Having an enum over a bunch of booleans not only makes it much clearer what the branches are, but also make a lot of weird combinations (like optional py/varargs, required kwargs, ...) unrepresentable, which is always good. This also moves the error checking of those cases to construction/parsing side instead of usage side, which for me feels a more appropriate place for them. In some places the matching is bit more verbose, but maybe there could be some helper methods for those (or maybe its even fine like it is)
There was a problem hiding this comment.
Great! In which case let's merge this one now so that it's possible to start reviewing the follow up :)
I took a shot at the refactoring of the
#[setter]argument discussed in #3998 (comment).This tries to decouples
impl_arg_paramfrom the layout used in regular#[pyfunction]/#[pymethods]by moving theargs_arrayassumption one layer up. I leftPropertyType::Descriptoruntouched because I don't see a good may to integrate that (but since that one is not customizable it seem fine)The deprecation warning make this still quite noisy, but I think once we can finally (🙏 ) remove them, this should get significantly better.