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

Is Gin Context violating the golang context styleguide? #4040

Open
xr opened this issue Aug 26, 2024 · 3 comments
Open

Is Gin Context violating the golang context styleguide? #4040

xr opened this issue Aug 26, 2024 · 3 comments

Comments

@xr
Copy link

xr commented Aug 26, 2024

Hey guys,

We're having some discussions regarding whether or not Gin Context violates the Go context style guide. I'd like to hear your thoughts on this.

According to the Go style guide (reference: Go Style Guide on Custom Contexts), it clearly states:

Do not create custom context types or use interfaces other than context.Context in function signatures. There are no exceptions to this rule.

However, in the case of Gin's HandlerFunc, it passes a custom *Context type.

Even though the Gin Context implements all the methods of the context.Context interface,

Deadline() (deadline time.Time, ok bool)
Done() <-chan struct{}
Err() error
Value(key any) any

as shown here

We still need to interpret the Go style guide. There seem to be two possible views on this:

  1. Gin's Context is acceptable: Since it implements the context.Context interface, it can be considered valid, and thus doesn't violate the style guide. Similarly, we could create other types like this:

    type interface CustomContext {
        context.Context
        // ...additional APIs that we need
    }
  2. Strict compliance: To adhere to the Go style guide strictly, we should always pass context.Context directly in function signatures. In this case, the design would need to change to separate the Context from custom APIs. For example, the function signature could be:

    type HandlerFunc func(ctx context.Context, api CustomAPIs)

What do you guys think?

@FarmerChillax
Copy link
Contributor

FarmerChillax commented Aug 28, 2024

As much as I want to upvote Strict compliance, but this is Break change. Unrealistic :p

some case:

@xr xr closed this as completed Sep 5, 2024
@mgerasimchuk
Copy link

@xr I think it would be better to keep this issue in open state as an improvement reminder for the next major version of the gin

it's extremely disorienting that I must not use the gin.Context and need to use (*gin.Context).Request.Context() instead

PS:
I faced the similar situation with otel like @pepea23 in #3993, and when I found that my otel.Span just disappeared from the context I was very surprised

PPS:
Another funny thing that there is no one sample in the gin's documentation with the context usage in a case when we should pass it deeper to other components. I think it should the very first line in the README, for real

@xr
Copy link
Author

xr commented Sep 16, 2024

@mgerasimchuk sure, i just reopen this issues, and fyi, cross posting the discussions in the style guideline repo: google/styleguide#850

@xr xr reopened this Sep 16, 2024
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

No branches or pull requests

3 participants