search

FDio / govpp

Go toolset for the VPP.
Apache License 2.0
193 stars 82 forks source link

Move plugins to own package & improve apigen CLI #29

Closed sknat closed 1 year ago

sknat commented 2 years ago

This patch moves the binapigenerator plugins to their own package.

This patch also proposes an evolution to the binapigen CLI in order to make it more user-friendly for newcomers as well as build pipelines based on go:generate

For now two usecases are in the picture:

Signed-off-by: Nathan Skrzypczak nathan.skrzypczak@gmail.com Change-Id: I5b0b2ade40ab80c9e91c2a422f8c193b232d9830

ondrej-fabry commented 1 year ago

Why including quite unrelated changes into here? It would be good to keep this PR only about separating the plugins - making the generator extendable. The changes to the API are not needed for this.

sknat commented 1 year ago

You mean having this patch on top of #28 or the fact that the 62f5161 in itself is too big in your view ? Regarding the former, I'm keeping those two stacked as we are depending on the combination, and I wanted avoid the perpetual rebase overhead.

ondrej-fabry commented 1 year ago

You mean having this patch on top of #28 or the fact that the 62f5161 in itself is too big in your view ? Regarding the former, I'm keeping those two stacked as we are depending on the combination, and I wanted avoid the perpetual rebase overhead.

I think both. Basically I think a change like this should be done in multiple steps. This helps with doing reviews, bisecting possible issues and makes the changes testable separately.

Overall, I do not see how these changes relate together in GoVPP and I believe these considerations must have priority over dependency/requirements of other projects.

sknat commented 1 year ago

Ok, so I did split this patch & the previous one, we have (in order)

All these patches are on top of each other (it's way easier due to dependencies between each of them), so looking at a PR will include changes in the previous ones until merged & rebased