Standard library routing #151
Conversation
| } | ||
|
|
||
| serveFromFS, err := strconv.ParseBool(os.Getenv("SERVE_FROM_FS")) |
There was a problem hiding this comment.
Consider lifting this parsing of the env vars out of the server and have it get passed in as an option in NewServer
There was a problem hiding this comment.
I'll do this neatly in its own PR so I don't have to look for it later
|
|
||
| //TODO: Add sample logs and metrics | ||
| writer.WriteHeader(http.StatusOK) | ||
| _, isCI := os.LookupEnv("CI") |
There was a problem hiding this comment.
Ditto for this env var, maybe an "autoOpenBrowser" option, or have the app code itself open the browser instead of the server
Definitely not necessary to change in this PR though
| writer.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| log.Fatal(err) |
There was a problem hiding this comment.
Consider instrumentation / logging instead of immediately failing. We could create a single function to handle errors that cannot be recovered from so they are consistent.
| return s.server.Close() | ||
| } | ||
|
|
||
| func (s *Server) Handler(serveFromFS bool) http.Handler { |
There was a problem hiding this comment.
serveFromFS should probably also be an option passed into NewServer.
Naively, I would expect the routes to be set up as part of NewServer not require a further call
There was a problem hiding this comment.
I'm not sure where else to handle this one, since serveFromFS is only used during development. I'll figure it out in the same PR as the others though.
server.goto use standard routing instead of gorilla/mux (thank you for your service)store.goandqueries.go