Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(86)

Issue 6852075: code review 6852075: go/format: Package format implements standard formattin... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by gri
Modified:
9 years, 7 months ago
Reviewers:
bradfitz, rsc, rog
CC:
r, bradfitz, dfc, rog, rsc, golang-dev
Visibility:
Public.

Description

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.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+338 lines, -52 lines) Patch
M src/cmd/fix/main.go View 1 2 3 4 10 11 3 chunks +4 lines, -17 lines 0 comments Download
M src/cmd/godoc/godoc.go View 1 2 3 4 10 11 2 chunks +2 lines, -3 lines 0 comments Download
M src/cmd/godoc/play.go View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -28 lines 0 comments Download
M src/cmd/godoc/template.go View 1 2 3 4 2 chunks +4 lines, -4 lines 0 comments Download
A src/pkg/go/format/format.go View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +200 lines, -0 lines 0 comments Download
A src/pkg/go/format/format_test.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +125 lines, -0 lines 0 comments Download

Messages

Total messages: 25
gri
Hello r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
9 years, 7 months ago (2012-11-21 01:37:31 UTC) #1
bradfitz
Nice. On Tue, Nov 20, 2012 at 5:37 PM, <gri@golang.org> wrote: > Reviewers: r, > ...
9 years, 7 months ago (2012-11-21 02:04:13 UTC) #2
dfc
nit: please fix the typo in the CL description. On Wed, Nov 21, 2012 at ...
9 years, 7 months ago (2012-11-21 02:04:54 UTC) #3
gri
Hello r@golang.org, bradfitz@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-21 02:18:58 UTC) #4
gri
PS: Better (more usable, simpler) API suggestions welcome. - gri On Tue, Nov 20, 2012 ...
9 years, 7 months ago (2012-11-21 02:20:08 UTC) #5
bradfitz
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 ...
9 years, 7 months ago (2012-11-21 03:27:14 UTC) #6
r
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 ...
9 years, 7 months ago (2012-11-21 03:42:27 UTC) #7
rog
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 ...
9 years, 7 months ago (2012-11-21 15:01:39 UTC) #8
gri
Hello r@golang.org, bradfitz@golang.org, dave@cheney.net, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-21 23:04:45 UTC) #9
r
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 ...
9 years, 7 months ago (2012-11-22 00:31:42 UTC) #10
gri
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: > ...
9 years, 7 months ago (2012-11-22 01:08:20 UTC) #11
rog
LGTM with one thought about the name. Also I wonder if you might want to ...
9 years, 7 months ago (2012-11-22 09:41:46 UTC) #12
rsc
Looks pretty good. I haven't read the whole conversation, but I like the new name ...
9 years, 7 months ago (2012-11-25 15:35:12 UTC) #13
gri
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: ...
9 years, 7 months ago (2012-11-26 23:18:49 UTC) #14
bradfitz
I thought the whole point of this package was so callers didn't have to know ...
9 years, 7 months ago (2012-11-26 23:40:34 UTC) #15
gri
If you use format.Source it does sort imports and you don't have to know anything ...
9 years, 7 months ago (2012-11-27 01:09:26 UTC) #16
bradfitz
It doesn't seem worth distinguishing the two cases. I'd write a tiny amount of code ...
9 years, 7 months ago (2012-11-27 01:33:59 UTC) #17
gri
Fair enough. PTAL. - gri On Mon, Nov 26, 2012 at 5:33 PM, Brad Fitzpatrick ...
9 years, 7 months ago (2012-11-27 02:28:10 UTC) #18
bradfitz
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#newcode56 src/pkg/go/format/format.go:56: panic("unreachable") sure? might as well just "return err".
9 years, 7 months ago (2012-11-27 02:33:18 UTC) #19
gri
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? ...
9 years, 7 months ago (2012-11-27 06:22:54 UTC) #20
bradfitz
LGTM On Mon, Nov 26, 2012 at 10:22 PM, <gri@golang.org> wrote: > PTAL. > > ...
9 years, 7 months ago (2012-11-27 06:27:35 UTC) #21
rsc
LGTM But please start on a CL that makes this more efficient. We can do ...
9 years, 7 months ago (2012-11-27 15:06:47 UTC) #22
rog
LGTM On 27 November 2012 15:06, <rsc@golang.org> wrote: > LGTM > > But please start ...
9 years, 7 months ago (2012-11-27 15:21:23 UTC) #23
rsc
> agreed. an companion to ast.Walk that allowed selective > mutation of the ast nodes ...
9 years, 7 months ago (2012-11-27 15:31:22 UTC) #24
gri
9 years, 7 months ago (2012-11-27 18:29:56 UTC) #25
*** 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b