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

Issue 12837044: code review 12837044: cmd/gofmt: sort more, remove some duplicate imports (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by josharian
Modified:
10 years, 6 months ago
Reviewers:
gri, rsc
CC:
rsc, golang-dev
Visibility:
Public.

Description

cmd/gofmt: sort more, remove some duplicate imports * Sort imports by import path, then import name, then comment. Currently, gofmt sorts only by import path. * If two imports have the same import path and import name, and one of them has no comment, remove the import with no comment. (See the discussion at issue 4414.) Based on @rsc's https://codereview.appspot.com/7231070/ Fixes issue 4414.

Patch Set 1 #

Patch Set 2 : diff -r 9f6a7e4c3f62 https://code.google.com/p/go #

Patch Set 3 : diff -r ec64f75a7995 https://code.google.com/p/go #

Patch Set 4 : diff -r ec64f75a7995 https://code.google.com/p/go #

Patch Set 5 : diff -r ec64f75a7995 https://code.google.com/p/go #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -18 lines) Patch
M src/cmd/gofmt/testdata/import.golden View 1 1 chunk +18 lines, -0 lines 0 comments Download
M src/cmd/gofmt/testdata/import.input View 1 1 chunk +23 lines, -0 lines 0 comments Download
M src/pkg/go/ast/import.go View 1 2 5 chunks +80 lines, -18 lines 0 comments Download
M src/pkg/go/token/position.go View 1 2 3 1 chunk +9 lines, -0 lines 2 comments Download

Messages

Total messages: 9
josharian
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
10 years, 7 months ago (2013-08-27 18:05:45 UTC) #1
josharian
On 2013/08/27 18:05:45, josharian wrote: > Hello mailto:rsc@golang.org (cc: mailto:golang-dev@googlegroups.com), > > I'd like you ...
10 years, 7 months ago (2013-08-27 18:07:14 UTC) #2
r
10 years, 7 months ago (2013-08-28 01:20:26 UTC) #3
rsc
LGTM I hope that when gri is back he will comment on token.(*File).RemoveLine, but for ...
10 years, 6 months ago (2013-09-06 20:15:03 UTC) #4
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=f09e38f4b079 *** cmd/gofmt: sort more, remove some duplicate imports * Sort imports ...
10 years, 6 months ago (2013-09-06 20:25:18 UTC) #5
gri
https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go File src/pkg/go/token/position.go (right): https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go#newcode139 src/pkg/go/token/position.go:139: // RemoveLine removes a line by line number as ...
10 years, 6 months ago (2013-09-10 20:51:05 UTC) #6
rsc
Can you send a CL? Thanks. Russ
10 years, 6 months ago (2013-09-10 20:57:48 UTC) #7
josharian
rsc wrote: "Can you send a CL?" I'm not sure whether "you" refers to me ...
10 years, 6 months ago (2013-09-10 23:34:49 UTC) #8
gri
10 years, 6 months ago (2013-09-10 23:45:24 UTC) #9
I think rsc meant me, but please feel free to take a crack at it. Please
send the review directly to me.

I think MergeLines with one argument is fine. Or MergeLine (singular), as
in merge this line with the next one. Or JoinLine. We can debate a better
name if one comes up.

The table is defined to contain the offset of the first character for each
line, so the first entry is always 0 as you observed. I'm not saying the
code is wrong, but by the comment, valid input line numbers should start at
1 (currently, 0 would not crash, I think). Maybe that requires a +1, -1
that cancel each other out, but at least then then we have a match between
API documentation and behavior.

Hope that helps.
- gri



On Tue, Sep 10, 2013 at 4:34 PM, <josharian@gmail.com> wrote:

> rsc wrote: "Can you send a CL?"
>
> I'm not sure whether "you" refers to me or gri, but I'd be happy to send
> a cleanup CL. The only outstanding question, to my mind, is the proper
> name of the function; see my inline reply. I'd love input.
>
> Thanks for the careful review, gri, much appreciated.
>
> -josh
>
>
>
>
> https://codereview.appspot.**com/12837044/diff/13001/src/**
>
pkg/go/token/position.go<https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go>
> File src/pkg/go/token/position.go (right):
>
> https://codereview.appspot.**com/12837044/diff/13001/src/**
>
pkg/go/token/position.go#**newcode139<https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go#newcode139>
> src/pkg/go/token/position.go:**139: // RemoveLine removes a line by line
> number as reported by Position.Line.
>
>> RemoveLine is slightly misleading a name.
>>
>
> Agreed. I started with RemoveNewline, was unsure about its clarity, and
> failed to come up with a better alternative.
>
>
>
>  MergeLines comes to mind since this merges two lines - but doesn't
>>
> really
>
>> change the subsequent position offsets.
>>
>
> I thought about MergeLines, but that feels like it should take two
> parameters. Other suggestions?
>
>
>
>  The comments should be adjusted as well.
>>
>
> Yes.
>
>
>
>  Also, lines are numbered starting at 1 while the table is 0-indexed;
>>
> there seems
>
>> to be an off-by-1 error somewhere; or the comment needs to be
>>
> adjusted. An input
>
>> of 0 should not be accepted, either.
>>
>
> When I looked at this, the first entry in the table was always 0; in
> practice, the code below is (I believe) correct and does match the doc.
> I assumed that that had been done to avoid having to think about
> off-by-1 errors when manipulating the table, and I appreciated it at the
> time!
>
>
https://codereview.appspot.**com/12837044/<https://codereview.appspot.com/128...
>
Sign in to reply to this message.

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