Add support for max-count param in /api/runs #298
Conversation
lukebjerring
force-pushed
the
max-count-param
branch
from
ad2cbcb
to
4bd7feb
Compare
|
A description in the first commit / description would be very nice, both for context when reviewing and when finding this in git blame later. |
|
Commit history cleaned up, PTAL |
| baseQuery := datastore.NewQuery("TestRun").Order("-CreatedAt").Limit(1) | ||
| var limit int | ||
| if limit, err = ParseMaxCountParam(r); err != nil { | ||
| http.Error(w, "Invalid 'max-count' param: "+err.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
Aside: I don't know Go, but am wondering if runtime error exist, and if so how easy/hard it is to write error handling code that's untested and will actually blow up in a different way than intended if run. (Reminds me of a C++ code base I worked on with a lot of out-of-memory handling that was untested.)
There was a problem hiding this comment.
Some reading for your leisure time: https://blog.golang.org/defer-panic-and-recover
params.go
Outdated
| // ParseMaxCountParam parses the 'max-count' parameter as an integer. | ||
| func ParseMaxCountParam(r *http.Request) (count int, err error) { | ||
| count = MaxCountDefaultValue | ||
| if browsersParam := r.URL.Query().Get("max-count"); browsersParam != "" { |
There was a problem hiding this comment.
Should this by maxCountParam or countParam instead of browsersParam?
There was a problem hiding this comment.
Fixed (CopyPasta)
| } | ||
|
|
||
| func TestParseMaxCountParam_TooSmall(t *testing.T) { | ||
| r := httptest.NewRequest("GET", "http://wpt.fyi/?max-count=0", nil) |
There was a problem hiding this comment.
Also try max-count=-1 or max-count=1e8 to see that the "not a non-negative integer" error handling is right?
util.go
Outdated
| @@ -47,6 +47,11 @@ func GetBrowsers() (map[string]Browser, error) { | |||
| // GetBrowserNames returns an alphabetically-ordered array of the names | |||
| // of the browsers returned by GetBrowsers. | |||
| func GetBrowserNames() ([]string, error) { | |||
| if browserNamesAlphabetical == nil { | |||
There was a problem hiding this comment.
Is this change accidentally in this PR, or was it a necessary adjustment?
lukebjerring
force-pushed
the
max-count-param
branch
from
4bd7feb
to
6724132
Compare
See #301