Issue 6852075: code review 6852075: go/format: Package format implements standard formattin... (Closed)
Descriptiongo/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.
Patch Set 1 #Patch Set 2 : diff -r 1315abc581ed https://code.google.com/p/go #Patch Set 3 : diff -r 1315abc581ed https://code.google.com/p/go #Patch Set 4 : diff -r 1315abc581ed https://code.google.com/p/go #
Total comments: 10
Patch Set 5 : diff -r f2755950769b https://code.google.com/p/go #
Total comments: 1
Patch Set 6 : diff -r f2755950769b https://code.google.com/p/go #Patch Set 7 : diff -r f2755950769b https://code.google.com/p/go #
Total comments: 8
Patch Set 8 : diff -r 538f0e9733bc https://code.google.com/p/go #Patch Set 9 : diff -r 538f0e9733bc https://code.google.com/p/go #Patch Set 10 : diff -r 8f9a26de2b20 https://code.google.com/p/go #Patch Set 11 : diff -r 8f9a26de2b20 https://code.google.com/p/go #Patch Set 12 : diff -r 616adac0bb59 https://code.google.com/p/go #
Total comments: 2
Patch Set 13 : diff -r 616adac0bb59 https://code.google.com/p/go #Patch Set 14 : diff -r 7646c94159a1 https://code.google.com/p/go #
MessagesTotal messages: 25
Hello r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Nice. On Tue, Nov 20, 2012 at 5:37 PM, <gri@golang.org> wrote: > Reviewers: r, > > Message: > Hello r (cc: golang-dev@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > go/fmt: fmt implements standard formatting of Go surce > > Package fmt is a utility package that takes care of > parsing, sorting of imports, and formatting of .go > source files using the canonical gofmt formatting > parameters. > > Please review this at http://codereview.appspot.com/**6852075/<http://codereview.appspot.com/6852075/> > > Affected files: > A src/pkg/go/fmt/fmt.go > A src/pkg/go/fmt/fmt_test.go > > >
Sign in to reply to this message.
nit: please fix the typo in the CL description. On Wed, Nov 21, 2012 at 1:04 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Nice. > > > On Tue, Nov 20, 2012 at 5:37 PM, <gri@golang.org> wrote: >> >> Reviewers: r, >> >> Message: >> Hello r (cc: golang-dev@googlegroups.com), >> >> I'd like you to review this change to >> https://code.google.com/p/go >> >> >> Description: >> go/fmt: fmt implements standard formatting of Go surce >> >> Package fmt is a utility package that takes care of >> parsing, sorting of imports, and formatting of .go >> source files using the canonical gofmt formatting >> parameters. >> >> Please review this at http://codereview.appspot.com/6852075/ >> >> Affected files: >> A src/pkg/go/fmt/fmt.go >> A src/pkg/go/fmt/fmt_test.go >> >> >
Sign in to reply to this message.
Hello r@golang.org, bradfitz@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
PS: Better (more usable, simpler) API suggestions welcome. - gri On Tue, Nov 20, 2012 at 6:18 PM, <gri@golang.org> wrote: > Hello r@golang.org, bradfitz@golang.org, dave@cheney.net (cc: > golang-dev@googlegroups.com), > > Please take another look. > > > http://codereview.appspot.com/6852075/
Sign in to reply to this message.
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#newcode32 src/pkg/go/fmt/fmt.go:32: return (&printer.Config{Mode: printerMode, Tabwidth: tabWidth}).Fprint(dst, fset, node) this printer.Config could be a global variable. 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) { Could also make src be of type interface{}, like parser.ParseFile, which takes []byte, string, or io.Reader.
Sign in to reply to this message.
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 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.
Sign in to reply to this message.
nice. 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#newcode36 src/pkg/go/fmt/fmt.go:36: // an error. src is expected to be a syntactically correct Go source file. It would be more useful if this implemented the same heuristics that gofmt does to allow formatting of more kinds of program elements. 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. 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.
Sign in to reply to this message.
Hello r@golang.org, bradfitz@golang.org, dave@cheney.net, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/6852075/diff/1004/src/pkg/go/format/format_tes... File src/pkg/go/format/format_test.go (right): https://codereview.appspot.com/6852075/diff/1004/src/pkg/go/format/format_tes... src/pkg/go/format/format_test.go:86: // TODO(gri) Test formatting of partial propgrams. a partial propgram is called a program?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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#... 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.
Sign in to reply to this message.
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#... 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#... 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) ?
Sign in to reply to this message.
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#... 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#... 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#... 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.
Sign in to reply to this message.
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, <gri@golang.org> wrote: > PTAL. > Thanks. > > > > https://codereview.appspot.**com/6852075/diff/1007/src/cmd/**fix/main.go<http... > 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/6852... >
Sign in to reply to this message.
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, <gri@golang.org> 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#... >> 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#... >> 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#... >> 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/ > >
Sign in to reply to this message.
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 <gri@golang.org> 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, <gri@golang.org> 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#... > >> 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#... > >> 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#... > >> 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/ > > > > >
Sign in to reply to this message.
Fair enough. PTAL. - gri On Mon, Nov 26, 2012 at 5:33 PM, Brad Fitzpatrick <bradfitz@golang.org> 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 <gri@golang.org> 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, <gri@golang.org> 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#... >> >> 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#... >> >> 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#... >> >> 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/ >> > >> > > >
Sign in to reply to this message.
LGTM 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... src/pkg/go/format/format.go:56: panic("unreachable") sure? might as well just "return err".
Sign in to reply to this message.
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... 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.
Sign in to reply to this message.
LGTM On Mon, Nov 26, 2012 at 10:22 PM, <gri@golang.org> 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/6852... >
Sign in to reply to this message.
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
Sign in to reply to this message.
LGTM On 27 November 2012 15:06, <rsc@golang.org> 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.
Sign in to reply to this message.
> 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
Sign in to reply to this message.
*** 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
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
