code review 165051: use new tab indentation format in godoc, but convert

use new tab indentation format in godoc, but convert
leading tabs into spaces to ensure a good outcome in
most browsers

[gri, 2009-12-04 01:37:35]

Hello rsc,

I'd like you to review the following change.

[gri, 2009-12-04 21:15:19]

If you haven't started looking at this, please wait. I think this can
be done better and simpler.
- Robert

On Thursday, December 3, 2009,  <gri@golang.org> wrote:
> Reviewers: rsc,
>
> Message:
> Hello rsc,
>
> I'd like you to review the following change.
>
>
> Description:
> use new tab indentation format in godoc, but convert
> leading tabs into spaces to ensure a good outcome in
> most browsers
>
> Please review this at http://codereview.appspot.com/165051
>
> Affected files:
>   M src/cmd/godoc/godoc.go
>
>
> Index: src/cmd/godoc/godoc.go
> ===================================================================
> --- a/src/cmd/godoc/godoc.go
> +++ b/src/cmd/godoc/godoc.go
> @@ -454,15 +454,82 @@
>
>
>  //
----------------------------------------------------------------------------
> +// Tab conversion
> +
> +var spaces = strings.Bytes("                ") // 16 spaces
> +
> +const (
> +       indenting       = iota;
> +       collecting;
> +)
> +
> +// A tconv is an io.Writer filter for converting leading tabs into spaces.
> +type tconv struct {
> +       output  io.Writer;
> +       state   int;    // indenting or collecting
> +       indent  int;    // valid if state == indenting
> +}
> +
> +
> +func (p *tconv) writeIndent(n int) (err os.Error) {
> +       i := n * *tabwidth;
> +       for i > len(spaces) {
> +               i -= len(spaces);
> +               if _, err = p.output.Write(spaces); err != nil {
> +                       return
> +               }
> +       }
> +       _, err = p.output.Write(spaces[0:i]);
> +       return;
> +}
> +
> +
> +func (p *tconv) Write(data []byte) (n int, err os.Error) {
> +       pos := 0;       // valid if p.state == collecting
> +       var b byte;
> +       for n, b = range data {
> +               switch p.state {
> +               case indenting:
> +                       if b == '\t' {
> +                               p.indent++
> +                       } else {
> +                               p.state = collecting;
> +                               pos = n;
> +                               if err = p.writeIndent(p.indent); err != nil {
> +                                       return
> +                               }
> +                       }
> +               case collecting:
> +                       if b == '\n' {
> +                               p.state = indenting;
> +                               p.indent = 0;
> +                               if _, err = p.output.Write(data[pos : n+1]);
err != nil {
> +                                       return
> +                               }
> +                       }
> +               }
> +       }
> +       if p.state == collecting {
> +               _, err = p.output.Write(data[pos:])
> +       }
> +       return;
> +}
> +
> +
> +//
----------------------------------------------------------------------------
>  // Templates
>
>  // Write an AST-node to w; optionally html-escaped.
>  func writeNode(w io.Writer, node interface{}, html bool, styler
printer.Styler) {
> -       mode := printer.UseSpaces;
> +       mode := printer.TabIndent | printer.UseSpaces;
>         if html {
>                 mode |= printer.GenHTML
>         }
> -       (&printer.Config{mode, *tabwidth, styler}).Fprint(w, node);
> +       // convert trailing tabs into spaces using a tconv filter
> +       // to ensure a good outcome in most browsers (there may still
> +       // be tabs in comments and strings, but converting those into
> +       // the right number of spaces is much harder)
> +       (&printer.Config{mode, *tabwidth, styler}).Fprint(&tconv{output: w},
node);
>  }
>
>
>
>
>

[rsc, 2009-12-04 21:20:56]

ok - waiting

[rsc, 2009-12-13 20:41:24]

still waiting?

[gri, 2009-12-14 05:56:29]

yes - not convinced it's necessary for godoc, also it's not urgent
- robert

On Sun, Dec 13, 2009 at 12:41 PM, <rsc@golang.org> wrote:

> still waiting?
>
>
> http://codereview.appspot.com/165051
>

[gri, 2009-12-17 17:47:22]

PTAL.

I think its fine for now.

On Sun, Dec 13, 2009 at 12:41 PM, <rsc@golang.org> wrote:

> still waiting?
>
>
> http://codereview.appspot.com/165051
>

[rsc, 2009-12-17 20:31:28]

LGTM

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

http://codereview.appspot.com/165051/diff/4001/5001#newcode490
src/cmd/godoc/godoc.go:490: for n, b = range data {
for n,b is cute but doesn't work right:
at the end of the loop n=len(data)-1.

[gri, 2009-12-17 22:10:30]

fixed and submitted

On Thu, Dec 17, 2009 at 12:31 PM, <rsc@golang.org> wrote:

> LGTM
>
>
>
> http://codereview.appspot.com/165051/diff/4001/5001
> File src/cmd/godoc/godoc.go (right):
>
> http://codereview.appspot.com/165051/diff/4001/5001#newcode490
> src/cmd/godoc/godoc.go:490: for n, b = range data {
> for n,b is cute but doesn't work right:
> at the end of the loop n=len(data)-1.
>
>
> http://codereview.appspot.com/165051
>

[gri, 2009-12-17 22:10:52]

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

use new tab indentation format in godoc, but convert
leading tabs into spaces to ensure a good outcome in
most browsers

R=rsc
http://codereview.appspot.com/165051