Refactor server to use the Echo framework #26
Conversation
Makefile
Outdated
| @@ -41,7 +41,7 @@ vendor: | |||
| install-pact: | |||
| @if [ ! -d ./pact ]; then \ | |||
| echo "pact not installed, installing..."; \ | |||
| wget --quiet https://github.com/pact-foundation/pact-ruby-standalone/releases/download/v${PACT_VERSION}/${PACT_FILE} -O /tmp/pactserver.tar.gz && tar -xzf /tmp/pactserver.tar.gz 2>/dev/null -C .; \ | |||
| curl -s https://github.com/pact-foundation/pact-ruby-standalone/releases/download/v${PACT_VERSION}/${PACT_FILE} -L -o /tmp/pactserver.tar.gz && tar -xzf /tmp/pactserver.tar.gz 2>/dev/null -C .; \ | |||
There was a problem hiding this comment.
While we're changing this, can we do -sSLf
-s, --silent
-S, --show-error
When used with -s, --silent, it makes curl show an error message if it fails.
-L, --location
-f, --fail
perhaps all the options directly after curl, rather it being in the middle? -o could remain where it is I guess.
There was a problem hiding this comment.
Reading through the PR, I'm not sure how much Echo is actually contributing in making the code more readable. It is nice, but is it worth introducing it as a dependency? I do like the fact that we relied on http.Server before. What are your thoughts on this?
We've seen a few number of HTTP packages go out of date over time, such as Gorilla and now we're considering Echo over Gin etc etc. Is it worth making ourselves dependent on Echo given that we've already got the bindings. handlers etc setup?
| } | ||
|
|
There was a problem hiding this comment.
This routing is a good improvement in my opinion, defining routes by method makes it nice and clear what's going on.
|
|
||
| constraint, err := LoadConstraint(constraintBytes) | ||
| func (a *api) interactionsConstraintsHandler(c echo.Context) error { | ||
| constraint := interactionConstraint{} |
There was a problem hiding this comment.
Bind (unmarshal) into structs is another thing I like - just a helper function but works well
| httpresponse.Errorf(res, http.StatusBadRequest, "unable to read modifier. %s", err.Error()) | ||
| return | ||
| } | ||
| return c.NoContent(http.StatusOK) |
There was a problem hiding this comment.
Returning NoContent helps clarify expected response - current code hides this and it's not clear what response is sent
| } | ||
|
|
||
| func (a *api) sessionHandler(res http.ResponseWriter, req *http.Request) { | ||
| if req.Method == http.MethodDelete { |
There was a problem hiding this comment.
Getting rid of these if req.Method ... is a big win for me. I know it's only a small thing but it seems clumsy.
| } | ||
|
|
||
| func (a *api) interactionsWaitHandler(res http.ResponseWriter, req *http.Request) { | ||
| waitForCount, err := strconv.Atoi(req.URL.Query().Get("count")) |
There was a problem hiding this comment.
Looks like we don't check the http method at all in this handler? Easy to do, since it's not checked by the router. I'd say it's a GET since we're using query params, but looks like a POST or DELETE etc would also hit this code?
|
I'd agree it's mostly just boiler plate, and how we configure the server etc is no big deal. What is more important in terms of adding new features is making the handlers easy to work with. I've added a few comments inline to show where I think some of the wins are. If it would make it easier to get approval we could switch to gin instead of echo. TBH I see echo as a natural upgrade from gin, they're very similar, for me it just has a few useful improvements. |
|
You convinced me, I think we would be able to migrate to what ever comes next after Echo as well as the interfaces would be similar. Could we do the curl fix, and I then can approve if you don't mind |
Work towards of investment time project: https://github.com/form3tech/investment-time-project/issues/62
The Echo framework provides simple reusable code to make the setup, routing and handler code cleaner and more readable.
Echo is a good choice as it is fast, mature and stable. It is very similar to gin, but has a newer code base, is a more active project, has a few improvements particularly around routing and error handling.