Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use absolute imports, so nsq is go gettable #167

Merged
merged 1 commit into from
Mar 23, 2013
Merged

use absolute imports, so nsq is go gettable #167

merged 1 commit into from
Mar 23, 2013

Conversation

fsouza
Copy link
Contributor

@fsouza fsouza commented Mar 23, 2013

Fixes issue #166.

@mreiferson
Copy link
Member

Looks like the travis build is failing. You'll need to add the rest of the go get dependencies to .travis.yml

@fsouza
Copy link
Contributor Author

fsouza commented Mar 23, 2013

Currently, when travis-ci'ing go repositories, we set GOPATH and copy the code to it. Would it be good for you or do you prefer that I just add go get github.com/bitly/nsq/... to .travis.yml?

Here is an example:

@mreiferson
Copy link
Member

It probably makes sense to build the correct $GOPATH tree and copy code over to it.

Also, this section https://github.com/bitly/nsq/blob/master/INSTALLING.md#compiling needs updating. It should probably just read:


Use go get to download and compile the packages and binaries:

$ go get github.com/bitly/nsq/...

The Go package for building clients is github.com/bitly/nsq/nsq

@fsouza
Copy link
Contributor Author

fsouza commented Mar 23, 2013

Done, please take another look :-)

Here is the working build: https://travis-ci.org/fsouza/nsq/builds/5742716.

**assert** https://github.com/bmizerany/assert

$ go get github.com/bmizerany/assert

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like giving a shoutout to these other projects here 😄. Perhaps just remove the specific go get references for these two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looks like I'm stupid. Running go get github.com/bitly/nsq/... won't download assert because it's used only in tests.

Sorry about that, will fix.

@fsouza
Copy link
Contributor Author

fsouza commented Mar 23, 2013

Please take another look.

@mreiferson
Copy link
Member

Thanks @fsouza, looks good.

Want to add yourself to contributors on the README?

And finally, we like to rebase/squash down to 1 (occasionally more) logical commits before we merge... would you mind doing that?

@fsouza
Copy link
Contributor Author

fsouza commented Mar 23, 2013

Done, I reduced it to a single commit.

I've also ran gofmt -s -w ., please let me know if this does not look good :-)

@mreiferson
Copy link
Member

LGTM

thanks again!

mreiferson added a commit that referenced this pull request Mar 23, 2013
use absolute imports, so nsq is go gettable
@mreiferson mreiferson merged commit 22370af into nsqio:master Mar 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants