code review 4291059: godoc: add -template flag to specify custom templates

godoc: add -template flag to specify custom templates

[adg, 2011-03-21 04:35:53]

Hello gri (cc: golang-dev@googlegroups.com),

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

[niemeyer, 2011-03-21 13:06:15]

Hi Andrew,

I've also been thinking about something like this for a while.
My plan was a little different, though.  I was thinking of having
an optional lookup dictionary which enabled replacing *some*
templates, but would still fallback to the default ones if the
provided directory doesn't contain the requested one.

This would make it easier to do a "facelift" on a godoc website
while still allowing the unrelated content to change freely without
manual action.

What do you think?

[rsc1, 2011-03-21 13:51:52]

Even ignoring Gustavo's comment (which I think
has some merit) this is not right.

flag.String("template", filepath.Join(runtime.GOROOT(), "lib",
"godoc"), "directory containing godoc template files")

The default will not be affected by setting -goroot,
and it should be.  Better to have an empty string
default and then set it explicitly (if still empty)
after flag.Parse, using *goroot.

Russ

[gri, 2011-03-21 18:22:43]

I like this suggestion.
- Robert

On Mon, Mar 21, 2011 at 6:06 AM,  <n13m3y3r@gmail.com> wrote:
> Hi Andrew,
>
> I've also been thinking about something like this for a while.
> My plan was a little different, though.  I was thinking of having
> an optional lookup dictionary which enabled replacing *some*
> templates, but would still fallback to the default ones if the
> provided directory doesn't contain the requested one.
>
> This would make it easier to do a "facelift" on a godoc website
> while still allowing the unrelated content to change freely without
> manual action.
>
> What do you think?
>
> http://codereview.appspot.com/4291059/
>

[adg, 2011-03-21 22:20:47]

Hello gri, niemeyer, rsc1 (cc: golang-dev@googlegroups.com),

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

[adg, 2011-03-21 22:24:18]

A good suggestion that I've now implemented. PTAL.

On 22 March 2011 00:06,  <n13m3y3r@gmail.com> wrote:
> Hi Andrew,
>
> I've also been thinking about something like this for a while.
> My plan was a little different, though.  I was thinking of having
> an optional lookup dictionary which enabled replacing *some*
> templates, but would still fallback to the default ones if the
> provided directory doesn't contain the requested one.
>
> This would make it easier to do a "facelift" on a godoc website
> while still allowing the unrelated content to change freely without
> manual action.
>
> What do you think?
>
> http://codereview.appspot.com/4291059/
>

[gri, 2011-03-21 22:25:41]

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go#newcode68
src/cmd/godoc/godoc.go:68: templateDir    = flag.String("template", "",
"directory containing godoc template files")
-templatedir or maybe -templates seems like a better name to me

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:639: if _, err := os.Stat(path); err != nil {
You should report a non-fatal error here, otherwise it will be confusing to
understand what happened in case of an error.

[gri, 2011-03-21 22:26:27]

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go#newcode68
src/cmd/godoc/godoc.go:68: templateDir    = flag.String("template", "",
"directory containing godoc template files")
containing alternative template files

[niemeyer, 2011-03-21 22:26:48]

Thanks Andrew.

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go#newcode68
src/cmd/godoc/godoc.go:68: templateDir    = flag.String("template", "",
"directory containing godoc template files")
s/godoc/custom godoc/?

http://codereview.appspot.com/4291059/diff/4001/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:638: path := filepath.Join(*templateDir, name)
This should check if *templateDir != "", to avoid weird results from looking at
the current directory.

LGTM otherwise.

[adg, 2011-03-22 06:09:39]

Hello gri, niemeyer, rsc1 (cc: golang-dev@googlegroups.com),

Please take another look.

[niemeyer, 2011-03-22 13:02:50]

LGTM

[gri, 2011-03-22 21:30:41]

LGTM

[adg, 2011-03-22 22:44:31]

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

godoc: add -template flag to specify custom templates

R=gri, niemeyer, rsc1
CC=golang-dev
http://codereview.appspot.com/4291059