code review 5461047: misc/dashboard: user interface

misc/dashboard/app/build/ui.go

 // Copyright 2011 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 // TODO(adg): packages at weekly/release
 // TODO(adg): some means to register new packages
 
 package build
 
 import (
@@ skipping 38 matching lines @@
         }
 
         p := &Pagination{}
         if len(commits) == commitsPerPage {
                 p.Next = page + 1
         }
         if page > 0 {
                 p.Prev = page - 1
                 p.HasPrev = true
         }
-        err = uiTemplate.Execute(w, struct {

[rsc, 2011/12/15 23:33:58]

The fields get named types.  The struct should have a named type too.
This is a monster of a statement.

[adg, 2011/12/15 23:48:16]

Done.

-                Commits    []*Commit
-                Builders   []string
-                TipState   []*PackageState
-                Pagination *Pagination
-        }{
-                commits, builders, tipState, p,
-        })
-        if err != nil {
+        data := &uiTemplateData{commits, builders, tipState, p}
+        if err := uiTemplate.Execute(w, data); err != nil {
                 logErr(w, r, err)
         }
 }
 
 type Pagination struct {
         Next, Prev int
         HasPrev    bool
 }
 
 // goCommits gets a slice of the latest Commits to the Go repository.
@@ skipping 59 matching lines @@
                 for _, r := range results {
                         ok = ok && r.OK
                 }
                 states = append(states, &PackageState{
                         pkg, commit, results, ok,
                 })
         }
         return states, nil
 }
 
-var uiTemplate = template.Must(template.New("ui").Funcs(

[rsc, 2011/12/15 23:33:58]

how about

var uiTemplate = template.Must(
    template.New("ui").
        Funcs(
            ...
        }).
        ParseFile("build/ui.html"),
)

Or else make the FuncMap its own var.

[adg, 2011/12/15 23:48:16]

Done.

-        template.FuncMap{
-                "builderTitle": builderTitle,
-                "timeString":   timeString,

[rsc, 2011/12/15 23:33:58]

A better name might be datastoreTime
Of course, you can also write {{.T.Time}} instead of {{.T | timeString}}
and delete the function entirely.

[adg, 2011/12/15 23:48:16]

Done.

-                "shortHash":    shortHash,
-                "repoURL":      repoURL,
-        }).ParseFile("build/ui.html"))
+type uiTemplateData struct {
+        Commits    []*Commit
+        Builders   []string
+        TipState   []*PackageState
+        Pagination *Pagination
+}
+
+var uiTemplate = template.Must(
+        template.New("ui").
+                Funcs(template.FuncMap{
+                        "builderTitle": builderTitle,
+                        "shortHash":    shortHash,
+                        "repoURL":      repoURL,
+                }).
+                ParseFile("build/ui.html"),
+)
 
 // builderTitle formats "linux-amd64-foo" as "linux amd64 foo".
 func builderTitle(s string) string {
         return strings.Replace(s, "-", " ", -1)
 }
-
-// timeString returns a datastore.Time in string form.
-func timeString(t datastore.Time) string { return t.Time().String() }
 
 // shortHash returns a the short version of a hash.
 func shortHash(hash string) string {
         if len(hash) > 12 {
                 hash = hash[:12]
         }
         return hash
 }
 
 // repoRe matches Google Code repositories and subrepositories (without paths).
 var repoRe = regexp.MustCompile(`^code\.google\.com/p/([a-z0-9\-]+)(\.[a-z0-9\-]+)?$`)
 
 // repoURL returns the URL of a change at a Google Code repository or subrepo.
 func repoURL(hash, packagePath string) (string, os.Error) {
         if packagePath == "" {
                 return "https://code.google.com/p/go/source/detail?r=" + hash, nil
         }
         m := repoRe.FindStringSubmatch(packagePath)
         if m == nil {
                 return "", os.NewError("unrecognized package: " + packagePath)
         }
         url := "https://code.google.com/p/" + m[1] + "/source/detail?r=" + hash
         if len(m) > 2 {
                 url += "&repo=" + m[2][1:]
         }
         return url, nil
 }