code review 5991081: cmd/api: allow addition of API

cmd/api: allow addition of API

[bradfitz, 2012-04-10 13:24:36]

http://codereview.appspot.com/5991081/diff/5/src/cmd/api/goapi.go
File src/cmd/api/goapi.go (right):

http://codereview.appspot.com/5991081/diff/5/src/cmd/api/goapi.go#newcode40
src/cmd/api/goapi.go:40: allowAddition = flag.Bool("allowaddition", false,
"allow addition of API")
I'm not even sure this is needed.  Let's just always have this behavior.

In the future (whenever Go 1.1 is out), we'll make checkFile be a
$delimiter-separated list of files to check, but the behavior will always be the
same: no removals/changes.

Maybe what we'll introduce later is the rule that the final filename in the list
is special in a -lockdown=true mode, where even additions aren't allowed.

But let's deal with that later.

For now let's just make this do what we need today.

[rsc, 2012-04-10 13:37:42]

Agree with Brad: drop the flag here, then this is just a 1-line CL.

[minux1, 2012-04-10 14:00:43]

Hello bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2012-04-10 14:04:22]

LGTM

On Tue, Apr 10, 2012 at 7:00 AM, <minux.ma@gmail.com> wrote:

> Reviewers: bradfitz, rsc,
>
> Message:
> Hello bradfitz@golang.org, rsc@golang.org (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/api: allow addition of API
>
> Please review this at
http://codereview.appspot.com/**5991081/<http://codereview.appspot.com/5991081/>
>
> Affected files:
>  M src/cmd/api/goapi.go
>
>
> Index: src/cmd/api/goapi.go
> ==============================**==============================**=======
> --- a/src/cmd/api/goapi.go
> +++ b/src/cmd/api/goapi.go
> @@ -147,7 +147,7 @@
>                                changes = true
>                        case len(v1) == 0 || v1[0] > v2[0]:
>                                fmt.Fprintf(bw, "+%s\n", take(&v2))
> -                               changes = true
> +                               // we allow API additions now
>                        default:
>                                take(&v1)
>                                take(&v2)
>
>
>

[rsc, 2012-04-10 14:06:17]

LGTM

[minux1, 2012-04-10 14:12:40]

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

cmd/api: allow addition of API

R=bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/5991081

[rsc, 2012-04-10 15:14:35]

For the record, the build now says:

# Checking API compatibility.
+pkg syscall (windows-386), func GetCurrentProcessId() uint32
+pkg syscall (windows-amd64), func GetCurrentProcessId() uint32

which is exactly what I wanted to see.  (Please don't remove those lines, future
viewers of this CL!)

[r, 2012-04-10 21:25:56]

http://codereview.appspot.com/5991081/diff/2006/src/cmd/api/goapi.go
File src/cmd/api/goapi.go (right):

http://codereview.appspot.com/5991081/diff/2006/src/cmd/api/goapi.go#newcode150
src/cmd/api/goapi.go:150: // we allow API additions now
I'm not entirely happy with this change. We should allow additions, yes, but it
seems a missed opportunity to hide the changes we see. We could print them, but
that will get noisy. We could keep a secondary file, but that's messy. We could
change the API file, but that requires maintenance. Maybe there is no good
answer, but doing nothing is not the right choice.

I'd vote for changing the file. API changes are rare so the burden isn't too
great, but then history of the api file will tell me what we did when, much
easier than digging it out of the full log of the repo.

[bradfitz, 2012-04-10 21:28:59]

I thought we were going to start a new file now.

The plus lines will show until they've been vetted into the latest API file.

We never touch go1.txt at this point.  We touch go-next.txt.


On Tue, Apr 10, 2012 at 2:25 PM, <r@golang.org> wrote:

>
>
http://codereview.appspot.com/**5991081/diff/2006/src/cmd/api/**goapi.go<http...
> File src/cmd/api/goapi.go (right):
>
> http://codereview.appspot.com/**5991081/diff/2006/src/cmd/api/**
>
goapi.go#newcode150<http://codereview.appspot.com/5991081/diff/2006/src/cmd/api/goapi.go#newcode150>
> src/cmd/api/goapi.go:150: // we allow API additions now
> I'm not entirely happy with this change. We should allow additions, yes,
> but it seems a missed opportunity to hide the changes we see. We could
> print them, but that will get noisy. We could keep a secondary file, but
> that's messy. We could change the API file, but that requires
> maintenance. Maybe there is no good answer, but doing nothing is not the
> right choice.
>
> I'd vote for changing the file. API changes are rare so the burden isn't
> too great, but then history of the api file will tell me what we did
> when, much easier than digging it out of the full log of the repo.
>
>
http://codereview.appspot.com/**5991081/<http://codereview.appspot.com/5991081/>
>

[r2, 2012-04-10 21:38:09]

On 11/04/2012, at 7:28 AM, Brad Fitzpatrick wrote:

> I thought we were going to start a new file now.
> 
> The plus lines will show until they've been vetted into the latest API file.
> 
> We never touch go1.txt at this point.  We touch go-next.txt.

sgtm