FAQ
Hello r@golang.org, bradfitz@golang.org, dave@cheney.net,
rogpeppe@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.


http://codereview.appspot.com/6852075/

Search Discussions

  • R at Nov 22, 2012 at 12:31 am
    https://codereview.appspot.com/6852075/diff/1004/src/pkg/go/format/format_test.go
    File src/pkg/go/format/format_test.go (right):

    https://codereview.appspot.com/6852075/diff/1004/src/pkg/go/format/format_test.go#newcode86
    src/pkg/go/format/format_test.go:86: // TODO(gri) Test formatting of
    partial propgrams.
    a partial propgram is called a program?

    https://codereview.appspot.com/6852075/
  • Gri at Nov 22, 2012 at 1:08 am
    PTAL.


    https://codereview.appspot.com/6852075/diff/9001/src/pkg/go/fmt/fmt.go
    File src/pkg/go/fmt/fmt.go (right):

    https://codereview.appspot.com/6852075/diff/9001/src/pkg/go/fmt/fmt.go#newcode6
    src/pkg/go/fmt/fmt.go:6: package fmt
    On 2012/11/21 03:42:27, r wrote:
    s/surce/source/
    also in CL description
    this should be package format, not fmt. it will require a renamed
    import almost
    everywhere it's used.
    Done.

    https://codereview.appspot.com/6852075/diff/9001/src/pkg/go/fmt/fmt.go#newcode32
    src/pkg/go/fmt/fmt.go:32: return (&printer.Config{Mode: printerMode,
    Tabwidth: tabWidth}).Fprint(dst, fset, node)
    On 2012/11/21 03:27:14, bradfitz wrote:
    this printer.Config could be a global variable.
    Done.

    https://codereview.appspot.com/6852075/diff/9001/src/pkg/go/fmt/fmt.go#newcode36
    src/pkg/go/fmt/fmt.go:36: // an error. src is expected to be a
    syntactically correct Go source file.
    On 2012/11/21 15:01:39, rog wrote:
    It would be more useful if this implemented the same heuristics that
    gofmt does
    to allow formatting of more kinds of program elements.
    Agreed.

    https://codereview.appspot.com/6852075/diff/9001/src/pkg/go/fmt/fmt.go#newcode38
    src/pkg/go/fmt/fmt.go:38: func String(src string) (string, error) {
    On 2012/11/21 03:27:14, bradfitz wrote:
    Could also make src be of type interface{}, like parser.ParseFile,
    which takes
    []byte, string, or io.Reader.
    see comment below

    https://codereview.appspot.com/6852075/diff/9001/src/pkg/go/fmt/fmt.go#newcode38
    src/pkg/go/fmt/fmt.go:38: func String(src string) (string, error) {
    On 2012/11/21 15:01:39, rog wrote:
    On 2012/11/21 03:27:14, bradfitz wrote:
    Could also make src be of type interface{}, like parser.ParseFile,
    which takes
    []byte, string, or io.Reader.
    personally I'm not keen on that kind of thing. given that ParseFile
    always reads
    into a []byte anyway, perhaps just:
    func Write(dst io.Writer, source []byte) error
    might be good enough to replace both String and Copy here.
    the following doesn't seem too much worse than format.String(s)
    to me:
    var b bytes.Buffer
    err := format.Write(&b, []byte(s))
    use(b.String())
    doesn't seem too much worse than using format.String
    to me.
    The single entry point makes it obvious what the trade-offs are
    too.
    Agreed. The interface{} argument for ParseFile was an experiment. It
    didn't fail, but it was also not providing an overwhelmingly strong
    benefit.

    I kept Bytes (as in format.Bytes) but now using io.Writer and a []byte
    argument.

    I kept String (as in format.String) as a convenience function, and it
    turns out, it is actually used in that form already.

    https://codereview.appspot.com/6852075/
  • Rogpeppe at Nov 22, 2012 at 9:41 am
    LGTM with one thought about the name.

    Also I wonder if you might want to consider applying the same
    indentation heuristics as gofmt (for source fragments,
    it applies the indentation found on the first line to all
    subsequent lines).

    It would be nice if format.String(s) would be the same as piping the
    string through gofmt. Then the comment at the top would be strictly
    accurate.


    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go
    File src/pkg/go/format/format.go (right):

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    i'm not entirely sure about Bytes as a name here. I'd usually expect
    Bytes to return []byte, not write some bytes to a Writer.

    WriteTo would be more conventional.
    or even just Format.

    https://codereview.appspot.com/6852075/
  • Rsc at Nov 25, 2012 at 3:35 pm
    Looks pretty good. I haven't read the whole conversation, but I like the
    new name go/format (as compared to go/fmt). Two minor API concerns.



    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go
    File src/cmd/fix/main.go (left):

    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go#oldcode114
    src/cmd/fix/main.go:114: ast.SortImports(fset, f)
    Where did this call go? I am worried about putting it in format.Node,
    because that means format.Node is mutating its argument. Maybe there
    should be a separate format.Rewrite that does the mutating if that's
    indeed where it went.

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go
    File src/pkg/go/format/format.go (right):

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode31
    src/pkg/go/format/format.go:31: ast.SortImports(fset, file)
    This is problematic (see last comment).
    I would be happy to just move SortImports calls out to the callers.

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/22 09:41:47, rog wrote:
    i'm not entirely sure about Bytes as a name here. I'd usually expect Bytes to
    return []byte, not write some bytes to a Writer.
    WriteTo would be more conventional.
    or even just Format.
    I agree that the use of Bytes and String are unconventional here,
    although I do understand the reason for them. It also bothers me a
    little that Bytes and String have different signatures other than
    s/[]byte/string/. Perhaps we can solve both problems by merging the two
    functions into a single

    func Source(in []byte) (out []byte, err error)

    ?

    https://codereview.appspot.com/6852075/
  • Gri at Nov 26, 2012 at 11:18 pm
    PTAL.
    Thanks.


    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go
    File src/cmd/fix/main.go (left):

    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go#oldcode114
    src/cmd/fix/main.go:114: ast.SortImports(fset, f)
    On 2012/11/25 15:35:13, rsc wrote:
    Where did this call go? I am worried about putting it in format.Node, because
    that means format.Node is mutating its argument. Maybe there should be a
    separate format.Rewrite that does the mutating if that's indeed where
    it went.

    Done.

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go
    File src/pkg/go/format/format.go (right):

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode31
    src/pkg/go/format/format.go:31: ast.SortImports(fset, file)
    On 2012/11/25 15:35:13, rsc wrote:
    This is problematic (see last comment).
    I would be happy to just move SortImports calls out to the callers.
    Done.

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/22 09:41:47, rog wrote:
    i'm not entirely sure about Bytes as a name here. I'd usually expect Bytes to
    return []byte, not write some bytes to a Writer.
    WriteTo would be more conventional.
    or even just Format.
    ACK

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/25 15:35:13, rsc wrote:
    On 2012/11/22 09:41:47, rog wrote:
    i'm not entirely sure about Bytes as a name here. I'd usually expect
    Bytes to
    return []byte, not write some bytes to a Writer.

    WriteTo would be more conventional.
    or even just Format.
    I agree that the use of Bytes and String are unconventional here,
    although I do
    understand the reason for them. It also bothers me a little that Bytes and
    String have different signatures other than s/[]byte/string/. Perhaps we can
    solve both problems by merging the two functions into a single
    func Source(in []byte) (out []byte, err error)
    ?
    Done.

    https://codereview.appspot.com/6852075/
  • Brad Fitzpatrick at Nov 26, 2012 at 11:47 pm
    I thought the whole point of this package was so callers didn't have to
    know the fmt rules or configuration.

    Yet this package doesn't sort imports.

    So I still have to know little details. Doesn't seem to be any better than
    the status quo.

    I understand not wanting to mutate arguments, but I'd prefer a
    behind-the-scenes clone+mutate rather than callers having to care about
    details like import sorting.

    On Mon, Nov 26, 2012 at 3:18 PM, wrote:

    PTAL.
    Thanks.



    https://codereview.appspot.**com/6852075/diff/1007/src/cmd/**fix/main.go<https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go>
    File src/cmd/fix/main.go (left):

    https://codereview.appspot.**com/6852075/diff/1007/src/cmd/**
    fix/main.go#oldcode114<https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go#oldcode114>
    src/cmd/fix/main.go:114: ast.SortImports(fset, f)
    On 2012/11/25 15:35:13, rsc wrote:

    Where did this call go? I am worried about putting it in format.Node, because
    that means format.Node is mutating its argument. Maybe there should be a
    separate format.Rewrite that does the mutating if that's indeed where
    it went.

    Done.


    https://codereview.appspot.**com/6852075/diff/1007/src/pkg/**
    go/format/format.go<https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go>
    File src/pkg/go/format/format.go (right):

    https://codereview.appspot.**com/6852075/diff/1007/src/pkg/**
    go/format/format.go#newcode31<https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode31>
    src/pkg/go/format/format.go:**31: ast.SortImports(fset, file)
    On 2012/11/25 15:35:13, rsc wrote:

    This is problematic (see last comment).
    I would be happy to just move SortImports calls out to the callers.
    Done.


    https://codereview.appspot.**com/6852075/diff/1007/src/pkg/**
    go/format/format.go#newcode40<https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40>
    src/pkg/go/format/format.go:**40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/22 09:41:47, rog wrote:

    i'm not entirely sure about Bytes as a name here. I'd usually expect Bytes to
    return []byte, not write some bytes to a Writer.
    WriteTo would be more conventional.
    or even just Format.
    ACK


    https://codereview.appspot.**com/6852075/diff/1007/src/pkg/**
    go/format/format.go#newcode40<https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40>
    src/pkg/go/format/format.go:**40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/25 15:35:13, rsc wrote:
    On 2012/11/22 09:41:47, rog wrote:
    i'm not entirely sure about Bytes as a name here. I'd usually expect
    Bytes to
    return []byte, not write some bytes to a Writer.

    WriteTo would be more conventional.
    or even just Format.
    I agree that the use of Bytes and String are unconventional here,

    although I do
    understand the reason for them. It also bothers me a little that Bytes and
    String have different signatures other than s/[]byte/string/. Perhaps we can
    solve both problems by merging the two functions into a single
    func Source(in []byte) (out []byte, err error)
    ?
    Done.

    https://codereview.appspot.**com/6852075/<https://codereview.appspot.com/6852075/>
  • Robert Griesemer at Nov 27, 2012 at 1:09 am
    If you use format.Source it does sort imports and you don't have to
    know anything else.

    If you use format.Node is does not. I have looked into perhaps having
    a non-destructive import sorting, but it would require yet another ast
    entry point or a copy of some code of sorts in format. I've concluded
    that for now, this is fine. When you deal with the AST directly, you
    have to know more stuff anyway.
    On Mon, Nov 26, 2012 at 3:40 PM, Brad Fitzpatrick wrote:
    I thought the whole point of this package was so callers didn't have to know
    the fmt rules or configuration.

    Yet this package doesn't sort imports.

    So I still have to know little details. Doesn't seem to be any better than
    the status quo.

    I understand not wanting to mutate arguments, but I'd prefer a
    behind-the-scenes clone+mutate rather than callers having to care about
    details like import sorting.

    On Mon, Nov 26, 2012 at 3:18 PM, wrote:

    PTAL.
    Thanks.



    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go
    File src/cmd/fix/main.go (left):


    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go#oldcode114
    src/cmd/fix/main.go:114: ast.SortImports(fset, f)
    On 2012/11/25 15:35:13, rsc wrote:

    Where did this call go? I am worried about putting it in format.Node, because
    that means format.Node is mutating its argument. Maybe there should be a
    separate format.Rewrite that does the mutating if that's indeed where
    it went.

    Done.



    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go
    File src/pkg/go/format/format.go (right):


    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode31
    src/pkg/go/format/format.go:31: ast.SortImports(fset, file)
    On 2012/11/25 15:35:13, rsc wrote:

    This is problematic (see last comment).
    I would be happy to just move SortImports calls out to the callers.

    Done.



    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/22 09:41:47, rog wrote:

    i'm not entirely sure about Bytes as a name here. I'd usually expect Bytes to
    return []byte, not write some bytes to a Writer.
    WriteTo would be more conventional.
    or even just Format.

    ACK



    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/25 15:35:13, rsc wrote:
    On 2012/11/22 09:41:47, rog wrote:
    i'm not entirely sure about Bytes as a name here. I'd usually expect
    Bytes to
    return []byte, not write some bytes to a Writer.

    WriteTo would be more conventional.
    or even just Format.
    I agree that the use of Bytes and String are unconventional here,
    although I do
    understand the reason for them. It also bothers me a little that Bytes and
    String have different signatures other than s/[]byte/string/. Perhaps we can
    solve both problems by merging the two functions into a single
    func Source(in []byte) (out []byte, err error)
    ?

    Done.

    https://codereview.appspot.com/6852075/
  • Brad Fitzpatrick at Nov 27, 2012 at 1:34 am
    It doesn't seem worth distinguishing the two cases. I'd write a tiny
    amount of code and delete docs and confusion.

    I would just make format.Node check if it's an *ast.File and, if so, print
    it all out to a []byte and re-parse it. Then keep the docs that say "Node
    does not modify node" but delete the part that says "specifically, it does
    not sort imports".

    If the ast can ever have a more efficient clone we can switch the format
    package to use that without changing the API.

    On Mon, Nov 26, 2012 at 5:09 PM, Robert Griesemer wrote:

    If you use format.Source it does sort imports and you don't have to
    know anything else.

    If you use format.Node is does not. I have looked into perhaps having
    a non-destructive import sorting, but it would require yet another ast
    entry point or a copy of some code of sorts in format. I've concluded
    that for now, this is fine. When you deal with the AST directly, you
    have to know more stuff anyway.
    On Mon, Nov 26, 2012 at 3:40 PM, Brad Fitzpatrick wrote:
    I thought the whole point of this package was so callers didn't have to know
    the fmt rules or configuration.

    Yet this package doesn't sort imports.

    So I still have to know little details. Doesn't seem to be any better than
    the status quo.

    I understand not wanting to mutate arguments, but I'd prefer a
    behind-the-scenes clone+mutate rather than callers having to care about
    details like import sorting.

    On Mon, Nov 26, 2012 at 3:18 PM, wrote:

    PTAL.
    Thanks.



    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go
    File src/cmd/fix/main.go (left):

    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go#oldcode114
    src/cmd/fix/main.go:114: ast.SortImports(fset, f)
    On 2012/11/25 15:35:13, rsc wrote:

    Where did this call go? I am worried about putting it in format.Node, because
    that means format.Node is mutating its argument. Maybe there should be a
    separate format.Rewrite that does the mutating if that's indeed where
    it went.

    Done.


    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go
    File src/pkg/go/format/format.go (right):

    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode31
    src/pkg/go/format/format.go:31: ast.SortImports(fset, file)
    On 2012/11/25 15:35:13, rsc wrote:

    This is problematic (see last comment).
    I would be happy to just move SortImports calls out to the callers.

    Done.


    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/22 09:41:47, rog wrote:

    i'm not entirely sure about Bytes as a name here. I'd usually expect Bytes to
    return []byte, not write some bytes to a Writer.
    WriteTo would be more conventional.
    or even just Format.

    ACK


    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/25 15:35:13, rsc wrote:
    On 2012/11/22 09:41:47, rog wrote:
    i'm not entirely sure about Bytes as a name here. I'd usually expect
    Bytes to
    return []byte, not write some bytes to a Writer.

    WriteTo would be more conventional.
    or even just Format.
    I agree that the use of Bytes and String are unconventional here,
    although I do
    understand the reason for them. It also bothers me a little that Bytes and
    String have different signatures other than s/[]byte/string/. Perhaps we can
    solve both problems by merging the two functions into a single
    func Source(in []byte) (out []byte, err error)
    ?

    Done.

    https://codereview.appspot.com/6852075/
  • Robert Griesemer at Nov 27, 2012 at 2:28 am
    Fair enough. PTAL.
    - gri
    On Mon, Nov 26, 2012 at 5:33 PM, Brad Fitzpatrick wrote:
    It doesn't seem worth distinguishing the two cases. I'd write a tiny amount
    of code and delete docs and confusion.

    I would just make format.Node check if it's an *ast.File and, if so, print
    it all out to a []byte and re-parse it. Then keep the docs that say "Node
    does not modify node" but delete the part that says "specifically, it does
    not sort imports".

    If the ast can ever have a more efficient clone we can switch the format
    package to use that without changing the API.

    On Mon, Nov 26, 2012 at 5:09 PM, Robert Griesemer wrote:

    If you use format.Source it does sort imports and you don't have to
    know anything else.

    If you use format.Node is does not. I have looked into perhaps having
    a non-destructive import sorting, but it would require yet another ast
    entry point or a copy of some code of sorts in format. I've concluded
    that for now, this is fine. When you deal with the AST directly, you
    have to know more stuff anyway.

    On Mon, Nov 26, 2012 at 3:40 PM, Brad Fitzpatrick <bradfitz@golang.org>
    wrote:
    I thought the whole point of this package was so callers didn't have to
    know
    the fmt rules or configuration.

    Yet this package doesn't sort imports.

    So I still have to know little details. Doesn't seem to be any better
    than
    the status quo.

    I understand not wanting to mutate arguments, but I'd prefer a
    behind-the-scenes clone+mutate rather than callers having to care about
    details like import sorting.

    On Mon, Nov 26, 2012 at 3:18 PM, wrote:

    PTAL.
    Thanks.



    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go
    File src/cmd/fix/main.go (left):



    https://codereview.appspot.com/6852075/diff/1007/src/cmd/fix/main.go#oldcode114
    src/cmd/fix/main.go:114: ast.SortImports(fset, f)
    On 2012/11/25 15:35:13, rsc wrote:

    Where did this call go? I am worried about putting it in format.Node, because
    that means format.Node is mutating its argument. Maybe there should be a
    separate format.Rewrite that does the mutating if that's indeed where
    it went.

    Done.




    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go
    File src/pkg/go/format/format.go (right):



    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode31
    src/pkg/go/format/format.go:31: ast.SortImports(fset, file)
    On 2012/11/25 15:35:13, rsc wrote:

    This is problematic (see last comment).
    I would be happy to just move SortImports calls out to the callers.

    Done.




    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/22 09:41:47, rog wrote:

    i'm not entirely sure about Bytes as a name here. I'd usually expect Bytes to
    return []byte, not write some bytes to a Writer.
    WriteTo would be more conventional.
    or even just Format.

    ACK




    https://codereview.appspot.com/6852075/diff/1007/src/pkg/go/format/format.go#newcode40
    src/pkg/go/format/format.go:40: func Bytes(dst io.Writer, src []byte)
    error {
    On 2012/11/25 15:35:13, rsc wrote:
    On 2012/11/22 09:41:47, rog wrote:
    i'm not entirely sure about Bytes as a name here. I'd usually expect
    Bytes to
    return []byte, not write some bytes to a Writer.

    WriteTo would be more conventional.
    or even just Format.
    I agree that the use of Bytes and String are unconventional here,
    although I do
    understand the reason for them. It also bothers me a little that Bytes and
    String have different signatures other than s/[]byte/string/. Perhaps we can
    solve both problems by merging the two functions into a single
    func Source(in []byte) (out []byte, err error)
    ?

    Done.

    https://codereview.appspot.com/6852075/
  • Bradfitz at Nov 27, 2012 at 2:33 am
  • Gri at Nov 27, 2012 at 6:22 am
    PTAL.



    https://codereview.appspot.com/6852075/diff/11021/src/pkg/go/format/format.go
    File src/pkg/go/format/format.go (right):

    https://codereview.appspot.com/6852075/diff/11021/src/pkg/go/format/format.go#newcode56
    src/pkg/go/format/format.go:56: panic("unreachable")
    On 2012/11/27 02:33:19, bradfitz wrote:
    sure? might as well just "return err".
    Done.

    https://codereview.appspot.com/6852075/
  • Brad Fitzpatrick at Nov 27, 2012 at 6:27 am
    LGTM
    On Mon, Nov 26, 2012 at 10:22 PM, wrote:

    PTAL.




    https://codereview.appspot.**com/6852075/diff/11021/src/**
    pkg/go/format/format.go<https://codereview.appspot.com/6852075/diff/11021/src/pkg/go/format/format.go>
    File src/pkg/go/format/format.go (right):

    https://codereview.appspot.**com/6852075/diff/11021/src/**
    pkg/go/format/format.go#**newcode56<https://codereview.appspot.com/6852075/diff/11021/src/pkg/go/format/format.go#newcode56>
    src/pkg/go/format/format.go:**56: panic("unreachable")
    On 2012/11/27 02:33:19, bradfitz wrote:

    sure? might as well just "return err".
    Done.

    https://codereview.appspot.**com/6852075/<https://codereview.appspot.com/6852075/>
  • Rsc at Nov 27, 2012 at 3:06 pm
    LGTM

    But please start on a CL that makes this more efficient.
    We can do much better than 2x.
    It should be possible to do a shallow clone of the File, copying just
    these parts:

    Decls
    GenDecl, only for token.IMPORT
    Specs
    ImportSpec
    Name
    Path
    Comments
    CommentGroup



    https://codereview.appspot.com/6852075/
  • Roger peppe at Nov 27, 2012 at 3:28 pm
    LGTM
    On 27 November 2012 15:06, wrote:
    LGTM

    But please start on a CL that makes this more efficient.
    We can do much better than 2x.
    agreed. an companion to ast.Walk that allowed selective
    mutation of the ast nodes would be a nice thing to have,
    assuming there's a nicely general way to implement it.
  • Russ Cox at Nov 27, 2012 at 3:59 pm

    agreed. an companion to ast.Walk that allowed selective
    mutation of the ast nodes would be a nice thing to have,
    assuming there's a nicely general way to implement it.
    maybe but i'd rather write the 20 lines of code to fix the performance
    problem here first
    and leave redesigning ast.Walk for another day.

    russ
  • Gri at Nov 27, 2012 at 6:29 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=b8c822e083cf ***

    go/format: Package format implements standard formatting of Go source.

    Package format is a utility package that takes care of
    parsing, sorting of imports, and formatting of .go source
    using the canonical gofmt formatting parameters.

    Use go/format in various clients instead of the lower-level components.

    R=r, bradfitz, dave, rogpeppe, rsc
    CC=golang-dev
    http://codereview.appspot.com/6852075


    http://codereview.appspot.com/6852075/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 21, '12 at 11:10p
activeNov 27, '12 at 6:29p
posts17
users5
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase