refactor app context to use composition #5
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
appcontext/app_context.go
Outdated
| type AppContext struct { | ||
| // Context for cancellation | ||
| ctx context.Context | ||
| type Context interface { |
There was a problem hiding this comment.
I don't get why you need an interface here ?
I mean you should explain it
There was a problem hiding this comment.
because context.Context is an interface
There was a problem hiding this comment.
To be honest, it does need to be an interface. I have updated it to be a simple struct. app context.Context.
There was a problem hiding this comment.
The other option is to add a static method where you must pass the context every time.
appcontext.SetUser(parentCtx, "userId")
userId, ok := appcontext.GetUser(parentCtx)
``
|
It sounds better. Now, next remark You have package apperror with Error I would like to suggest you to move both in package app, this way you have no "appwhatever.Whatever" the repetition seems strange to me |
That's a good point but the other end is that
|
|
Having 2 files in the same packages is not a big issue :P |
|
Here is a dump of the error reported by golangci-lint (with a lot of linters enabled) |
|
I have merged the Which linters are you using? I can see most errors are about "british" vs "american" english. |
I pushed #7 I didn't notice you were already using golangci and govulneck, I have never thought about running them directly via go run. Very interesting |
Talento90
force-pushed
the
refactor-app-context
branch
from
c2fbf52 to
90867bc
Compare
| } | ||
|
|
||
| // UserID returns the user id | ||
| func (sc *Context) UserID() (string, bool) { |
There was a problem hiding this comment.
Unsure why you return the bool about the presence, you are not doing it for TraceID
There was a problem hiding this comment.
The idea is that TraceID() always exist, so I set it in the New() if it is absent. To allow debugging easily.
| func NewContext(ctx context.Context) Context { | ||
| appCtx := Context{Context: ctx} | ||
|
|
||
| if _, ok := ctx.Value(TraceIDKey).(string); !ok { |
There was a problem hiding this comment.
I would have called TraceID, and define one if the if was == "", my problem is that you somehow define two places where you check if you have a context.
This comment is made on your current code.
I think you could also consider to do not play/check/set with traceid in app.NewContext, but to define one when none is present in GetTraceID.
It would also fix an issue that someone with your current GetTraceID implementation.
Someone could define a context like this
ctx : = app.Context{Context: context.Background()} // possible as app.Context cannot be nothing but Exported
traceID := ctx.TraceID() // this would be emptyThere was a problem hiding this comment.
If you never call the TraceID() and pass the context as a value instead of a reference, the TraceID value will be mismatched.
There was a problem hiding this comment.
Indeed, but the points I raised are still valid, no?
There was a problem hiding this comment.
Yes, I added in the constructor and getter but it does not feel right 🤔
app/error.go
Outdated
| ErrorConflict ErrorType = "conflict" | ||
| // Timeout error | ||
| ErrorTimeout ErrorType = "timeout" | ||
| // Cancelled error |
There was a problem hiding this comment.
You forgot to update the comment
It should be ErrorCancelled
While you did it for severity
|
LGTM thanks |
Description
Refactor
appcontext.Contextto use the composition pattern to include the nativecontext.Context.