-
Notifications
You must be signed in to change notification settings - Fork 26
Prepare Router skeleton #37
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
Conversation
server/handler/render.go
Outdated
type errObj struct { | ||
Title string `json:"title"` | ||
} | ||
// TODO: it should be private |
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.
imo, don't need to be private.
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 made it private because nobody from the outside of the package should write in the response; should it?
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 can easily imagine when we would use it.
For example, we would create an endpoint to download diff or file.
func GetFile(r, w) {
name = getFileName(r)
f, err := getFile(name)
if err != nil {
Write(w, Error{"file with name %s doesn't exist", name})
}
http.ServeContent(w, f)
}
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.
but... that endpoint should be a handler, right?
If so, it can use handler.write
too, as done by handler.OauthCallback
76ed472#diff-3ad2ebeb7ccf861129aa87b26fa1f1d8R31
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.
ahhhh. you moved it from serializer to handler. then all good!
server/serializer/serializers.go
Outdated
func (e httpError) Error() string { | ||
if msg := e.Title; msg != "" { | ||
return msg | ||
} else if msg = http.StatusText(e.Status); msg != "" { |
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.
for the future: you don't need to use else
if you return
before
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.
It was done to reuse the msg
implicit definition made by the first if
.
Anyhow, there is no necessity of reusing it, so I'll separate it into two different if
, and redefine the msg
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.
done by 39208c3
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 saw few issues (for example about logger), but we can fix it later. Good to go!!!!!
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.
LGTM |
c9a3151
to
9983b3f
Compare
I rebased 9983b3f but I couldn't merge because this PR depends on #43 I also added 4f445cd with |
Added after PR review: - Naming conventions as suggested by @smacker - Separate if/return/else as suggested by @smacker - Move assignmentRequest to the handler that uses it - Make the handler.write and serializer.countResponse private - Avoid nil serializer.Response - go-vet. Rename id>ID - go-vet. Avoid unkeyed structs
9983b3f
to
f815af0
Compare
supersedes #16
depends on #43
partially solves #3
This PR prepares the skeleton of the backend api serving mocks, using the requirements defined by #3, and following the agreements on the implementations as we talked irl
If approved, the things that will be done in the next PRs will be: