Refactor pyproto internals

[davidhewitt]

Just a reminder of an idea to refactor pyproto internals.

See the idea in the comment from #1093 (review)

[davidhewitt]

@dvermd following up from your Gitter question: I had some experiments but no branch which is worth pushing.

The main idea I was playing with is that we have an enum MethodProto which causes a lot of the pyproto code to be duplicated for each enum branch.

I was wondering if something like the following would be more flexible.

pub struct MethodProto {
    name: &'static str,
    proto: &'static str,
    receiver: SelfType,
    args: &[&'static str],         // names of all the arguments
    result: Option<&'static str>   // name of the return type, if any (I think only `Free` does not have)
}

Happy to answer more questions if you're interested in working on this!

[dvermd]

All the firsts fields of this new struct (name, proto, receiver, args) can be set in defs.rs while filling the Proto structs.

result however seems to be taken from the method signature.

Should this still be the case and the result field removed from the new MethodProto struct ?
Did you thought about "forcing" return type Proto structs definitions in defs.rs ?

[kngwyu]

BTW, isn't it nice to first resolve #1206, where Unary is replaced by Unary*? I have a working branch for that, though I only have changed basic protocol.

[davidhewitt]

BTW, isn't it nice to first resolve #1206, where Unary is replaced by Unary*?

I think it can be done either way around - this refactoring makes it quite easy because receiver in the defs will control what the self type should be. After #1206 we wouldn't need receiver in the defs at all.

result however seems to be taken from the method signature.

Should this still be the case and the result field removed from the new MethodProto struct ?
Did you thought about "forcing" return type Proto structs definitions in defs.rs ?

I think the link you give is for the return type - what the def needs is whether this trait has a return type at all (because there is MethodProto::Free which has no Result. So maybe a bool is best. Or alternatively maybe the traits with MethodProto::Free should be changed to allow returning either () or PyResult<()>.

[dvermd]

I've got most of the function ported, the remaining ones are those in MethodProto::Free.
I can work around __clear__ with your advises above.

The other one is __traverse__ that is also declared as MethodProto::Free but the trait defines it with 1 parameter after &'p self. Should this method be treated as if it were a Binary one ? What makes it specific and being a Free ?

[davidhewitt]

Awesome!

I think the only special thing about Free was no return type?

If you want to open what you've got as a PR, it'd be easier for me to read through the changes and answer any todos / questions directly on that...

[dvermd]

Here is the PR.

The question about __traverse__ being a Free is because the trait defines fn __traverse__(&'p self, visit: PyVisit<'_>) -> Result<(), PyTraverseError> which has a return type.