Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a struct from gin's RegisterHandlers instead of an interface #555

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ericvolp12
Copy link
Contributor

This change provides an alternative to #554 based on the discussions in #530.

For setting up a Gin engine, we want to handle the IRouter interface when registering handlers, but as this only thinly wraps a *gin.GroupRouter, we can return a *gin.GroupRouter from the registration function.

We could refactor the RegisterHandlers and RegisterHandlersWithOptions to just take a *gin.GroupRouter to begin with, I'm not really sure what we gain by using the IRouter interface here. Does anyone have opinions on doing that?

@ericvolp12
Copy link
Contributor Author

I included autogeneration of new examples via go generate directives in the gin example folder that were missing which is likely what lead to this breaking change being merged without notice. If the changes make the generated code change in a breaking way, it will fail in make test now when it tries to build the example packages. make test runs after make generate so we should catch breaking changes this way.

@k0mmsussert0d
Copy link

Hi! Sorry to nudge you like this, but could you provide any estimate on when this is going to be merged? #485 poses major discrepancy when using this generator. I can see a solution had already been implemented in the past, but got reverted later due to inconsistencies with usage examples. If that's the only issue, would it be much of a trouble for you to align the documentation to match the actual API?

Afaik this project had some breaking changes in the release a week or two ago, perhaps now it's a good time to carry this change out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants