code review 5461047: misc/dashboard: user interface

misc/dashboard: user interface

[adg, 2011-12-15 05:41:08]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[rsc, 2011-12-15 16:48:29]

Please make a pass through and add comments.

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:119: func (c *Commit) GetResult(builder,
goHash string) *Result {
s/Get//

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:130: func (c *Commit) GetResults(goHash
string) (results []*Result) {
s/Get//

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:230: func GetTag(c appengine.Context, tag
string) (*Tag, os.Error) {
Can probably choose a better Get-free name but I don't know
what this does.  Doc comment?

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:397: // GetPackages returns all non-Go
packages.
s/Get//

[adg, 2011-12-15 23:23:05]

PTAL

Added a whole bunch of comments.

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:119: func (c *Commit) GetResult(builder,
goHash string) *Result {
On 2011/12/15 16:48:29, rsc wrote:
> s/Get//

Done.

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:130: func (c *Commit) GetResults(goHash
string) (results []*Result) {
On 2011/12/15 16:48:29, rsc wrote:
> s/Get//

Done.

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:230: func GetTag(c appengine.Context, tag
string) (*Tag, os.Error) {
The datastore function is called Get, and that's all this function does, so I
think GetTag is a fine name.

http://codereview.appspot.com/5461047/diff/11009/misc/dashboard/app/build/bui...
misc/dashboard/app/build/build.go:397: // GetPackages returns all non-Go
packages.
On 2011/12/15 16:48:29, rsc wrote:
> s/Get//

Done.

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

LGTM

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui.go
File misc/dashboard/app/build/ui.go (right):

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui....
misc/dashboard/app/build/ui.go:59: err = uiTemplate.Execute(w, struct {
The fields get named types.  The struct should have a named type too.
This is a monster of a statement.

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui....
misc/dashboard/app/build/ui.go:147: var uiTemplate =
template.Must(template.New("ui").Funcs(
how about

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

Or else make the FuncMap its own var.

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui....
misc/dashboard/app/build/ui.go:150: "timeString":   timeString,
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]

*** Submitted as http://code.google.com/p/go/source/detail?r=974a5265f6aa ***

misc/dashboard: user interface

R=rsc
CC=golang-dev
http://codereview.appspot.com/5461047

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui.go
File misc/dashboard/app/build/ui.go (right):

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui....
misc/dashboard/app/build/ui.go:59: err = uiTemplate.Execute(w, struct {
On 2011/12/15 23:33:58, rsc wrote:
> The fields get named types.  The struct should have a named type too.
> This is a monster of a statement.

Done.

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui....
misc/dashboard/app/build/ui.go:147: var uiTemplate =
template.Must(template.New("ui").Funcs(
On 2011/12/15 23:33:58, rsc wrote:
> how about
> 
> var uiTemplate = template.Must(
>     template.New("ui").
>         Funcs(
>             ...
>         }).
>         ParseFile("build/ui.html"),
> )
> 
> Or else make the FuncMap its own var.

Done.

http://codereview.appspot.com/5461047/diff/12023/misc/dashboard/app/build/ui....
misc/dashboard/app/build/ui.go:150: "timeString":   timeString,
On 2011/12/15 23:33:58, rsc wrote:
> A better name might be datastoreTime
> Of course, you can also write {{.T.Time}} instead of {{.T | timeString}}
> and delete the function entirely.

Done.