Skip to content
This repository has been archived by the owner on Feb 19, 2019. It is now read-only.

Structs to Interfaces #70

Merged
merged 2 commits into from Mar 25, 2013
Merged

Conversation

jtacoma
Copy link
Collaborator

@jtacoma jtacoma commented Mar 21, 2013

This adds a command "gozmqfix" that will update code when there are backwards incompatible changes in the API of the "github.com/alecthomas/gozmq" package.

The code is copied from the Go project at version 1.0.3 with its licensing intact. Based on a little research into licensing, I've added a copyright line near the top of the LICENSE file. I am not a lawyer.

Install with go install github.com/alecthomas/gozmq/gozmqfix. Usage is essentially the same as for go fix.

Finally, change interfaces to structs!

This started with some global search and replace, then cleaned up as necessary to get things building and passing tests again. Updating existing code should be as simple as:

go install github.com/alecthomas/gozmq/gozmqfix
gozmqfix PATH...

I've tested this on the files in imatix/zguide/examples/Go and it works fine (though a couple examples are missing the time.Duration update at the moment).

This is a command that will update code when there are backwards
incompatible changes in the API of the "github.com/alecthomas/gozmq"
package.  This commit brings in the skeleton with no specific API
updates yet included.

The code is copied from the Go project at version 1.0.3 with its
licensing intact.  Based on a little research into licensing, I've added
a copyright line near the top of the LICENSE file.  I am not a lawyer.

Install with `go install github.com/alecthomas/gozmq/gozmqfix`.  Usage
is essentially the same as for `go fix`.
@jtacoma
Copy link
Collaborator Author

jtacoma commented Mar 21, 2013

I'd merge this right into master because I like the change, but it deserves more consideration than that. Does gozmqfix really belong as a subdirectory? Is the licensing ok? I'd be happy to adjust anything here to make the whole change palatable.

Maybe there's some reason we'd rather stick with interfaces than structs? I see interfaces as being useful primarily for supporting code that doesn't care which implementation it's using, and structs as desirable here because it would let us customize the 2.x and 3.x API in a nicer way (e.g. socket and context options).

I'll wait a week and if nobody posts any other comments here I'll go ahead and merge to master, then update the examples in imatix/zguide.

@jtacoma jtacoma mentioned this pull request Mar 21, 2013
@alecthomas
Copy link
Owner

This is a very cool idea. Have you run it over the examples in the zguide?

@jtacoma
Copy link
Collaborator Author

jtacoma commented Mar 22, 2013

Yep! There are some issues because not all of them are updated to use time.Duration. I did this:

% cd ~/gocode/src/github.com/alecthomas/gozmq && go install
% cd gozmqfix && go install
% cd ~/zguide && git pull && cd examples/Go
Already up-to-date.
% gozmqfix .
identity.go: fixed zmqstruct
lbbroker.go: fixed zmqstruct
mdbroker.go: fixed zmqstruct
mdcliapi.go: fixed zmqstruct
mdwrkapi.go: fixed zmqstruct
ppworker.go: fixed zmqstruct
% for g in `grep -l '^func main() {' *.go` ; do go build $g zhelpers.go ; done
# command-line-arguments
./mdbroker.go:81: undefined: MDPW_DISCONNECT
[...]

There are a bunch of "undefined" errors in mdbroker.go, mdclient.go, and mdworker.go that can be fixed by using a proper build command, and the rest are time.Duration-related.

I was just now trying to put together something for the time.Duration change but I just realized it's less than straightforward, and perhaps not reasonably doable for me, to figure out whether given code has already been updated manually.

I'm going to see about fixing those examples for duration, then it'll be easier to verify this...

@jtacoma
Copy link
Collaborator Author

jtacoma commented Mar 22, 2013

Alright, I submitted a pull request to the zguide. Once it's merged, this...

for g in `grep -l '^func main' *.go` ; do go build -tags zmq_3_x $g `grep -L '^func main' *.go` ; done

...will build everything except for a few "redeclared in this block" errors due to the way I'm building them. I'm going to look into fixing them up so any example can be built with a one-liner, without having to memorize or track down all the dependencies.

@jtacoma
Copy link
Collaborator Author

jtacoma commented Mar 22, 2013

Awesome! I fixed the pull request to use the conventions established in the C and Java examples directories and now everything builds just fine. After the pull request is merged, in the examples/Go directory, this is all it takes:

gozmqfix .
./build all

All the examples build without issue in both 2.2 and 3.2.2 :-) ...mind you, I haven't actually tested all the examples, but I think it's fair to say that's outside the scope of converting structs to interfaces.

@jtacoma
Copy link
Collaborator Author

jtacoma commented Mar 22, 2013

Ok, Pieter merged the zguide update for durations and easy building. I think this is ready too. Is anybody unsure about this?

@alecthomas
Copy link
Owner

One good test would be to revert the changes to the tests, the run it on the gozmq tests. If they pass before and after...

But otherwise, LGTM.

@alecthomas
Copy link
Owner

Can you also add something like the following to the top-level .godocdown.md? Immediately before the Installing section I think would be good.

Upgrading

GoZMQ has made some public changes that will break old code. Fortunately, we've also written a tool that will upgrade your code for you! Here's how to run it over your source (after making a backup of course):

go get github.com/alecthomas/gozmq/gozmqfix
cd $YOUR_SOURCE_DIR
gozmqfix .

@alecthomas
Copy link
Owner

PS. You can regenerate the README.md with godocdown github.com/alecthomas/gozmq > README.md

@jtacoma
Copy link
Collaborator Author

jtacoma commented Mar 24, 2013

Funny thing about that, I wrote it to check import statements to find the right local name for gozmq in a given file - so it only works on files that import gozmq, and does nothing to any files in the gozmq package itself. I can fiddle with them to put them outside just for this test though, it's a good idea. I'm mostly offline this weekend so I'll be getting around to this over the next day or two. If this transition goes well I'll put together a blog post or something about how to publish backwards incompatible changes.

Started with some global search and replace, then cleaned up as
necessary to get things building and passing tests again.

This change also includes a "zmqstructs" rewrite for gozmqfix.  Updating
existing code should be as simple as:

    go install github.com/alecthomas/gozmq/gozmqfix
    gozmqfix PATH...
jtacoma added a commit that referenced this pull request Mar 25, 2013
Publish the hidden zmqContext and zmqSocket structs as Context and Socket, replacing the Context and Socket interfaces.
@jtacoma jtacoma merged commit 05dbfb0 into alecthomas:master Mar 25, 2013
@jtacoma
Copy link
Collaborator Author

jtacoma commented Mar 25, 2013

I tried it out with the gozmq tests and found one case the "zmqstructs" fix missed. It was missed because I didn't scour the "go/ast" docs for other cases. Anyway, now I've done that, your suggested test works and a few other cases (notably maps, e.g. map[zmq.Socket]bool) are covered too.

@jtacoma jtacoma deleted the struct-to-interface branch March 25, 2013 20:58
@alecthomas
Copy link
Owner

Very very cool, thanks for doing this. I think it's a great solution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants