-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add OnError option for the http response writer #84
Conversation
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.
Looks good! I like keeping two separate functions. If there's a reason we want the response function to be able to use the error, I could see just having one function. I can't think of a reason though. 👍
basculehttp/constructor.go
Outdated
|
||
next.ServeHTTP(response, request.WithContext(ctx)) | ||
}) | ||
} | ||
|
||
func (c *constructor) error(logger log.Logger, e ErrorResponseReason, auth string, err error) { | ||
func (c *constructor) error(logger log.Logger, w http.ResponseWriter, e ErrorResponseReason, auth string, err error) { |
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.
I don't think we need the error function anymore with these changes? Aren't we just calling it once?
basculehttp/constructor.go
Outdated
|
||
urlVal := *request.URL // copy the URL before modifying it | ||
u, err := c.parseURL(&urlVal) | ||
authentication, errReason, err := c.authenticationOutput(logger, request) |
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.
Feel free to ignore this comment.
authentication
is kind of a long word for a variable; I would suggest just a
or auth
.
Codecov Report
@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 67.78% 68.02% +0.23%
==========================================
Files 26 27 +1
Lines 683 688 +5
==========================================
+ Hits 463 468 +5
Misses 201 201
Partials 19 19
Continue to review full report at Codecov.
|
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.
🎉
Kudos, SonarCloud Quality Gate passed! |
No description provided.