Feedback

[swinton]

Capturing the feedback kindly provided here by @WillAbides...

---

go-probot notes

---

go-probot is using an old version of "github.com/google/go-github". v17 is the last version that is incompatible with go modules. You can update to a newer version by running go get -u github.com/google/go-github/v32/github. Then replace github.com/google/go-github/github with github.com/google/go-github/v32/github in all your import statements.

A quirk of go modules is that all semantic versions that are v2 or higher require /v<major version> at the end of the module name.

---

github.com/gorilla/mux isn't doing anything for you that the standard library can't. An updated version of Start() that uses the standard library is:

func Start() {
	initialize()

	mux := http.NewServeMux()
	mux.HandleFunc("/", rootHandler(app))

	// Server
	log.Printf("Server running at: http://localhost:%d/\n", portVar)
	log.Fatal(http.ListenAndServe(fmt.Sprintf("127.0.0.1:%d", portVar), mux))
}

---

Avoid global variables like handlers in probot/handlers.go. In you might consider making handlers a field in App and adding an AddHandler method like this

// HandleEvent adds an event handler to your app 
func (a *App) HandleEvent(event string, handler eventHandler) {
	if a.handlers == nil {
		a.handlers = map[string]eventHandler{}
	}
	a.handlers[event] = handler
}

Then update rootHandler to use app.handlers instead of the global variable.

---

rootHandler is doing a bit too much. Consider breakins the handler it returns into multiple methods. It will be more readable and easier to test (see next section).

---

You will want some tests eventually. When you add them, consider using assertions from https://github.com/stretchr/testify. That makes test writing a bit easier. Unless there is a specific reason stay away from testify's suite package. I used that when I was new to go and it caused me to write overly complex tests.

When writing tests for http handlers, use https://golang.org/pkg/net/http/httptest/#Server. But you probably don't want to jump into testing the handlers immediatly though. Instead break the handlers up into smaller functions and unit test those. That will give you a better idea of how testing in go works.

---

Building configuration from environment variables in NewApp limits users to always using those environment variables to configure their probots. It also makes those environment variables part of go-probot's API. Consider removing NewApp and letting the user compose that configuration themselves.