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

Update context to have a generic method for getting request data. #3329

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

Conversation

cheikhshift
Copy link

Add a generic method GetAs to get request data.

  • With pull requests:
    • Open your pull request against master
    • Your pull request should have no more than two commits, if not you should squash them.
    • It should pass all tests in the available continuous integration systems such as GitHub Actions.
    • You should add/modify tests to cover your proposed code changes.
    • If your pull request contains a new feature, please document it on the README.

Add a generic method `GetAs` to get request data.
use method Get to get existing data.
@cheikhshift cheikhshift changed the title Update context to have a generic method. Update context to have a generic method for getting request data. Oct 5, 2022
@@ -265,6 +265,18 @@ func (c *Context) Get(key string) (value any, exists bool) {
return
}

// GetAs returns the value for the given key, ie: (value, true).
// If the value does not exist it returns (nil, false)
func (c *Context) GetAs[T any](key string) (result T, exists bool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need support go1.16 up version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be problematic, as Generics were introduced in Go 1.19. Any suggestions?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of this method over the non-generic version? The biggest issue with Get is that a failed type assertion causes a run-time panic, which seems to be the same here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to assert the variable to a certain type. The assertion is also checked and returns true/false depending on the case.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@appleboy gin requires go 1.21 or above now. Would you consider this feature again??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laggu Yes, we should reconsider the changes.

@Ozoniuss
Copy link

Ozoniuss commented Jun 20, 2023

I would find adding this method to be very useful, if support for an older Go version (perhaps in a new Gin release) would no longer be required. If passing custom types in the request context, it helps avoiding some of the code duplication incurred by using the ctx.Get() method followed by a type assertion.

To give a concrete example, in this project I am working on it would've helped to avoid having to define a custom function when retrieving context parameters that are of custom type. I could've used this GetAs in the same way I used GetInt. (code for reference)

id := ctx.GetInt("id")
req, ok := common.CtxGetTyped[casheerapi.UpdateEntryRequest](ctx, "req")
if !ok {
	return
}

With that method, I wouldn't have had to define my custom method CtxGetType which does the same as the new GetAs (actually it does more things for the sake of safety, but I could've omitted them because a middleware is validating the type beforehand).

What's the benefit of this method over the non-generic version? The biggest issue with Get is that a failed type assertion causes a run-time panic, which seems to be the same here?

Avoiding the code duplication by doing the type assertions yourself is a good enough benefit IMO. Functions like GetInt, GetBool, GetTime etc. that are already defined for the library do pretty much the same, except they convert the value to an existing type. If we have those, I would argue that this new function would be just as useful.

@appleboy
Copy link
Member

I think we don't need the changes from #3633 anymore.

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.

5 participants