-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
feat: gin.Context GetXxx added support for more go native types #3633
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should keep the same logic as the original
.Get
and returns a(int8, bool)
to avoid panicking in case the value if not of the asked type. This comment applies to other methods as well. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, I just noticed this problem too, but there is a small compatibility problem that seems strange, the methods I added this time are styled by the functions that already existed before:
and
The style is kept uniform, and these two functions seem to start from this PR:
Then other GetXxx functions such as:
It seems that they all follow the rule of only one return value.
All in all, there seem to be two styles for the Get method on the Context. The first one is:
Represented by two return values, the other is based on
represented by one return value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I didn't see that, so what you've done makes sense.
But for the record, I guess we could leverage Go generic with something like this:
Sure we can't add this on the
Context
type because Go doesn't currently support method level generic types yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you have a point, the best way is to add a generic method to Context, although I don't think Golang will support this feature any time soon, but I believe it will be supported in the future, it is a mature language should have a feature. some proposal about it:
golang/go#49085
However, in the absence of generics, enumeration method can only be used. Although there is a lot of repetitive work, using a separate method can solve this problem, but it will be a problem to promote the use of this function. Most people use the method in Context and then view it with dot. So I think it is necessary to add these methods to Context for now, as a transition that will remove in the future when Golang supports Struct generic methods.