Add support for specifying which browsers in /api/runs #280
Conversation
params.go
Outdated
| } | ||
|
|
||
| runParam := params.Get("sha") | ||
| regex := regexp.MustCompile("[0-9a-fA-F]{10}") |
There was a problem hiding this comment.
This could be a pre-compiled constant outside the func.
There was a problem hiding this comment.
Technically not a const, but exported to re-used var.
params.go
Outdated
| return name, nil | ||
| } | ||
| } | ||
| return "", nil |
params.go
Outdated
| if regex.MatchString(runParam) { | ||
| runSHA = runParam | ||
| } | ||
| return runSHA, err |
There was a problem hiding this comment.
Can we log here? No error will be reported but we're falling back on something other than what was input to the function.
There was a problem hiding this comment.
It's user-input, so maybe we should just throw an error. Either way - refactor or plumbing needed, so will follow-up. #283
| return nil, err | ||
| } | ||
| if len(browsers) == 0 { | ||
| return browserNames, nil |
| // Otherwise filter to valid browser names. | ||
| for i := 0; i < len(browsers); { | ||
| if !IsBrowserName(browsers[i]) { | ||
| // 'Remove' browser by switching to end and cropping. |
There was a problem hiding this comment.
If you say so 😃 @jeffcarp can you please golang-review this bit?
| // Otherwise filter to valid browser names. | ||
| for i := 0; i < len(browsers); { | ||
| if !IsBrowserName(browsers[i]) { | ||
| // 'Remove' browser by switching to end and cropping. |
There was a problem hiding this comment.
Changed #283 to include this.
params.go
Outdated
|
|
||
| // ParseBrowsersParam parses the 'browsers' parameter, split on commas. | ||
| // If empty, it also checks for the (repeatable) 'browser' params, before falling back to the default set of | ||
| // browsers. |
There was a problem hiding this comment.
Mention returns sorted list?
|
|
||
| func TestParseSHAParam_2(t *testing.T) { | ||
| sha := "0123456789" | ||
| r := httptest.NewRequest("GET", "http://wpt.fyi/?sha="+sha, nil) |
There was a problem hiding this comment.
These files are gofmt'd, that's the way they like it 😕
| @@ -52,7 +51,7 @@ func apiTestRunsHandler(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
|
|
|||
| var browserNames []string | |||
| if browserNames, err = GetBrowserNames(); err != nil { | |||
| if browserNames, err = ParseBrowsersParam(r); err != nil { | |||
There was a problem hiding this comment.
@jeffcarp is this special case of multiple-statements-on-one-line-with-error-checking a standard golang idiom?
There was a problem hiding this comment.
Yep, they're called 'short statements' - https://tour.golang.org/flowcontrol/6
Params handler works for both
and