chore(build): Refactor server trait gen to dedup some code

[yotamofek]

Motivation

A lot of the code for building the server trait is duplicated.

Solution

Refactor to reduce said duplication.

[tottoto]

I'd like to keep the current styles for the conditions about whether it is client streaming or server streaming or not. This refactoring is well designed to avoid reimplementing logic, but at the same time, I feel that it has become more difficult to grasp the outline of the processing from the perspective of code generation. Taking the trade-off into consideration, how about introducing body_or_semicolon to the conditional branch of generate_default_stubs to reduce the number of branches?

[yotamofek]

Yeah I agree, refactored a bit more to look more like the older code. There's a bit of duplication here but I think it's a reasonable trade-off because it makes the branching easier to follow. What do you think?

[tottoto]

What I tried to say is that using switch with client streaming and/or server streaming, and then in the each branch using body_or_semicolon variable to handle the default stub option.

And, I would like to write partial_sig and return_ty inline directly without making them variables.

[yotamofek]

Not sure I understand what you mean. I don't really see a point in doing something like match (method.client_streaming(), method.server_streaming()) since those two settings don't impact one another.

[tottoto]

If proper separation seems difficult, it seems likely that refactoring aimed at introducing further complexity will also be difficult.

[yotamofek]

@tottoto BTW, I think the whole server-trait generation logic could go in its own module and be split up into functions. I didn't want to make such an intrusive rewrite without seeing if the crate's maintainers are onboard with the idea, but while we're already cleaning this up - I can give it a go and see if it turns out well. If you think that's a good idea 😊

[tottoto]

@tottoto BTW, I think the whole server-trait generation logic could go in its own module and be split up into functions. I didn't want to make such an intrusive rewrite without seeing if the crate's maintainers are onboard with the idea, but while we're already cleaning this up - I can give it a go and see if it turns out well. If you think that's a good idea 😊

If a specific explanation of the proposal is provided, it might help understanding.

[yotamofek]

If a specific explanation of the proposal is provided, it might help understanding.

I don't haha, just thought I'd start by moving everything into a separate module and start splitting it up into smaller functions. Don't have a specific proposal in mind. I can take a crack at it at some point.

Though to be honest, not sure how much effort I'd wanna put into this, my main goal was to get #2115 in, that would be really helpful for my project @ work, this cleanup was meant to make that feature easier to develop.

[tottoto]

Though to be honest, not sure how much effort I'd wanna put into this, my main goal was to get #2115 in, that would be really helpful for my project @ work, this cleanup was meant to make that feature easier to develop.

Even if such a refactoring were to be merged, there is no guarantee that #2115 would be merged. That's a different discussion point.

[yotamofek]

Though to be honest, not sure how much effort I'd wanna put into this, my main goal was to get #2115 in, that would be really helpful for my project @ work, this cleanup was meant to make that feature easier to develop.

Even if such a refactoring were to be merged, there is no guarantee that #2115 would be merged. That's a different discussion point.

Yeah, I know, what I'm saying is I don't know how much effort I want to put into this PR because it won't help #2115 (or something similar) get merged.

[tottoto]

Yeah, I know, what I'm saying is I don't know how much effort I want to put into this PR because it won't help #2115 (or something similar) get merged.

This pull request is an extraction of a part from #2115 that seems complex and independently discussable and applicable, so it is essentially a part of it in the sense that it is a precursor to #2115. We had the option of reviewing it directly in #2115, but the inclusion of a different perspective, that of adding functionality, would make it more complicated, and since this part itself would be independently useful if successful, proposed splitting it into a separate it as this pull request for discussion.

As a result of the experiments and discussions here, it seems that refactoring is not that easy, so it will be difficult to apply #2115 while maintaining maintainability with the current design.

[yotamofek]

Refactoring is quite easy, agreeing on what constitutes "better" in matters of style and taste is futile :)

Feel free to take this PR and modify it in any way that suits you. I agree it's a good idea to review and discuss it separately. But seeing as you're not too inclined on adding the feature I suggested in #2115 (though I do think people other than me will find it beneficial), I don't have the motivation to spend more time on this, especially since it seems to me like the changes you're suggesting would be easier to make than describe.

[tottoto]

Refactoring is quite easy, agreeing on what constitutes "better" in matters of style and taste is futile :)

It is important to consider not only adding features but also future maintenance, so it is important to consider what kind of refactoring is appropriate. It is not a mere matter of style.

Feel free to take this PR and modify it in any way that suits you. I agree it's a good idea to review and discuss it separately. But seeing as you're not too inclined on adding the feature I suggested in #2115 (though I do think people other than me will #2115 (comment)), I don't have the motivation to spend more time on this, especially since it seems to me like the changes you're suggesting would be easier to make than describe.

Since this is your suggestion and you are not going to proceed it will be closed. Next time, be careful not to submit a pull request that you are not motivated to pursue yourself. The community has limited resources and no one has the resources to put effort into it.