Milo: Console improvements

milo/frontend/view_console.go

Index: milo/frontend/view_console.go
diff --git a/milo/frontend/view_console.go b/milo/frontend/view_console.go
index 595139531f699de21edf84ab68703b369ea84ba8..23236cb235da6ead971e1511c4e52067313470fd 100644
--- a/milo/frontend/view_console.go
+++ b/milo/frontend/view_console.go
@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"html/template"
 	"net/http"
+	"strings"
 
 	"golang.org/x/net/context"
 
@@ -49,33 +50,50 @@ func getConsoleDef(c context.Context, project, name string) (*common.Console, er
 	return cs, nil
 }
 
-func console(c context.Context, project, name string) (*resp.Console, error) {
+// shortname returns the short if given, otherwise tries to calculate
+// a short name out of a long.
+func shortname(short, long string) string {

[dnj, 2017/08/02 23:33:04]

This "short" logic is weird. IMO, just have the caller do:

name := meta.ShortName
if name == "" {
  name = shortname(b)
}

[dnj, 2017/08/02 23:33:04]

What is "short" and "long" expected to be? Can you detail this in the comment?

[Ryan Tseng, 2017/08/03 00:46:12]

Done.

+	if short != "" {
+		return short
+	}
+	builderName := strings.SplitN(long, "/", 3)[2]

[dnj, 2017/08/02 23:33:05]

This makes the assumption that "long" has at least two "/" in it. Let's test
length first here. Alternatively, you could do "strings.Split(...)" (not SplitN)
and get the last element. Or flatten the string if it doesn't have two
slashes...

[Ryan Tseng, 2017/08/03 00:46:12]

Done.

+	tokens := strings.FieldsFunc(builderName, func(r rune) bool {
+		switch r {
+		case '_', '-', ' ':
+			return true
+		}
+		return false
+	})
+	numLetters := len(tokens)
+	if numLetters > 3 {
+		numLetters = 3
+	}
+	for i := 0; i < numLetters; i++ {
+		short += string(tokens[i][0])

[dnj, 2017/08/02 23:33:05]

If the string has two delimiters (e.g., "--"), wouldn't one of the tokens be
empty? In this case, [0] will panic. Add logic to skip tokens[i] if
len(tokens[i]) == 0.

[dnj, 2017/08/02 23:33:05]

Why the string cast?

[Ryan Tseng, 2017/08/03 00:46:12]

Done.  And apparently the index of a string is a rune or byte....

+	}
+	return strings.ToLower(short)
+}
+
+func console(c context.Context, project, name string, limit int) (*resp.Console, error) {
 	tStart := clock.Now(c)
 	def, err := getConsoleDef(c, project, name)
 	if err != nil {
 		return nil, err
 	}
-	commitInfo, err := git.GetHistory(c, def.RepoURL, def.Ref, 25)
+	commitInfo, err := git.GetHistory(c, def.RepoURL, def.Ref, limit)
 	if err != nil {
 		return nil, err
 	}
 	tGitiles := clock.Now(c)
 	logging.Debugf(c, "Loading commits took %s.", tGitiles.Sub(tStart))
 
-	builderNames := make([]string, len(def.Builders))
-	builders := make([]resp.BuilderRef, len(def.Builders))
-	for i, b := range def.Builders {
-		builderNames[i] = b
-		builders[i].Name = b
-		_, _, builders[i].ShortName, _ = buildsource.BuilderID(b).Split()
-		// TODO(hinoka): Add Categories back in.
-	}
-
 	commitNames := make([]string, len(commitInfo.Commits))
 	for i, commit := range commitInfo.Commits {
 		commitNames[i] = hex.EncodeToString(commit.Hash)
 	}
-	rows, err := buildsource.GetConsoleRows(c, project, def, commitNames, builderNames)
+
+	builderRefs := make([]resp.BuilderRef, len(def.Builders))

[dnj, 2017/08/02 23:33:05]

Define this down near the "for" loop that uses it?

[Ryan Tseng, 2017/08/03 00:46:12]

Done.

+	rows, err := buildsource.GetConsoleRows(c, project, def, commitNames, def.Builders)
 	tConsole := clock.Now(c)
 	logging.Debugf(c, "Loading the console took a total of %s.", tConsole.Sub(tGitiles))
 	if err != nil {
@@ -84,7 +102,7 @@ func console(c context.Context, project, name string) (*resp.Console, error) {
 
 	ccb := make([]resp.CommitBuild, len(commitInfo.Commits))
 	for row, commit := range commitInfo.Commits {
-		ccb[row].Build = make([]*model.BuildSummary, len(builders))
+		ccb[row].Build = make([]*model.BuildSummary, len(def.Builders))
 		ccb[row].Commit = resp.Commit{
 			AuthorName:  commit.AuthorName,
 			AuthorEmail: commit.AuthorEmail,
@@ -95,8 +113,14 @@ func console(c context.Context, project, name string) (*resp.Console, error) {
 			Revision:    resp.NewLink(commitNames[row], def.RepoURL+"/+/"+commitNames[row]),
 		}
 
-		for col, b := range builders {
-			name := buildsource.BuilderID(b.Name)
+		for col, b := range def.Builders {
+			meta := def.BuilderMetas[col]
+			builderRefs[col] = resp.BuilderRef{
+				Name:      b,
+				Category:  strings.Split(meta.Category, "|"),
+				ShortName: shortname(meta.ShortName, b),
+			}
+			name := buildsource.BuilderID(b)
 			if summaries := rows[row].Builds[name]; len(summaries) > 0 {
 				ccb[row].Build[col] = summaries[0]
 			}
@@ -106,7 +130,7 @@ func console(c context.Context, project, name string) (*resp.Console, error) {
 	return &resp.Console{
 		Name:       def.ID,
 		Commit:     ccb,
-		BuilderRef: builders,
+		BuilderRef: builderRefs,
 	}, nil
 }
 
@@ -158,7 +182,13 @@ func (c consoleRenderer) Header() template.HTML {
 	// Last row: The actual builder shortnames.
 	result += "<tr><th></th><th></th>"

[dnj, 2017/08/02 23:33:04]

nit: for this sort of construction, it's better to:

must := func(v int, err error) {
  if err != nil {
    panic(err)
  }
}

var buf bytes.Buffer
must(buf.WriteString(...))
must(fmt.Fprintf(&buf, ...)
return template.HTML(buf.String())


waaaaay fewer buffer allocation/copy.

[Ryan Tseng, 2017/08/03 00:46:13]

Done.

 	for _, br := range c.BuilderRef {
-		result += fmt.Sprintf("<th>%s</th>", br.ShortName)
+		_, _, builderName, err := buildsource.BuilderID(br.Name).Split()
+		if err != nil {
+			builderName = err.Error()

[dnj, 2017/08/02 23:33:05]

This seems not very user-friendly :x Perhaps set the builder name to "ERR" and
have hovertext with the error body?

[Ryan Tseng, 2017/08/03 00:46:13]

Good idea. Done.

+		}
+		result += fmt.Sprintf(
+			"<th><a href=\"/%s\" title=\"%s\">%s</a></th>",
+			br.Name, builderName, br.ShortName)
 	}
 	result += "</tr>"
 	return template.HTML(result)
@@ -180,8 +210,12 @@ func ConsoleHandler(c *router.Context) {
 		return
 	}
 	name := c.Params.ByName("name")
+	limit := 25

[dnj, 2017/08/02 23:33:04]

Define this as a constant at the top?

[Ryan Tseng, 2017/08/03 00:46:12]

If it's not reused it's better to be defined closer to the code (M-A style
coding)

[dnj, 2017/08/03 01:02:58]

Meh fine, then you can inline:

const maxLimit = 1000
const defaultLimit = 25

...

+	if tLimit := GetLimit(c.Request, -1); tLimit >= 0 {
+		limit = tLimit

[dnj, 2017/08/02 23:33:05]

Should we apply an upper bound here?

[Ryan Tseng, 2017/08/03 00:46:12]

Okay 1000 it is

+	}
 
-	result, err := console(c.Context, project, name)
+	result, err := console(c.Context, project, name, limit)
 	if err != nil {
 		ErrorHandler(c, err)
 		return