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

Merging creack/cleanup - proposed v2 #8

Merged
merged 9 commits into from
Aug 6, 2015
Merged

Merging creack/cleanup - proposed v2 #8

merged 9 commits into from
Aug 6, 2015

Conversation

jmervine
Copy link
Contributor

@jmervine jmervine commented Aug 3, 2015

Replacing #3 (cc @jdorfman)

@creack Merged your branch here, with the following changes...

  • Adding Godeps to lock dependancies.
  • Ensuring go 1.3 support, as it's latest stable for Debian (which is what I use).
  • Adding LICENSE file (addressing Add LICENSE file #4)

Original PR notes:

Minor cleanup including:

  • Add all example to godoc (rename example function)
  • Discard travis tests for go < 1.4
  • Use .PHONY rule in makefile
  • comply to golint
  • Use "standalone" structs instead of nested for easier testing
  • Remove pointer on header because it is already a reference
  • use http.Dump{Request,Response} instead of json.MarshalIndent
  • remove unnecessary named returns
  • remove unnecessary mutexes
  • close channels when finished
  • minor optimizations
  • Add .gitignore

@jmervine jmervine mentioned this pull request Aug 3, 2015
@jmervine jmervine changed the title Merging creack/cleanup Merging creack/cleanup - proposed v2 Aug 3, 2015

"github.com/garyburd/go-oauth/oauth"
"github.com/MaxCDN/go-maxcdn/Godeps/_workspace/src/github.com/garyburd/go-oauth/oauth"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. it should still be "github.com/garyburd/go-oauth/oauth"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a choice in how you use Godeps, I typically use this method, opting not to change the GOPATH. That import line was written directly by Godeps itself. If we use Godeps at all, I greatly prefer this method.

@creack
Copy link
Contributor

creack commented Aug 3, 2015

The imports should be "regular", Godeps should not be part of the import paths.
Beside this: 👍

@jmervine
Copy link
Contributor Author

jmervine commented Aug 3, 2015

RE Godeps, can you provide a little insight as to why you prefer using standard import paths? I've only read the docs and not had a chance to discuss the pros and cons of both with anyone. It seemed to me that the method I used with better/simpler for both development and building, but I'd genuinely love to hear the other side of it.

Cheers,
J

@creack
Copy link
Contributor

creack commented Aug 3, 2015

Godep is a useful tool, but should be "hidden". The idea is to be 100% go with godep has a helper.

Forcing the import path to the godep subdir makes the import ugly but more importantly, forces the developer to use that version of the code. Which will be done anyway when using godep go build but having the "standard" import path allow the developer to use it's own version of the dependencies.

@jmervine
Copy link
Contributor Author

jmervine commented Aug 3, 2015

Interesting, okay. I'll play around with that. I know there's some changes you have to make to the way travis works when attempting to use Godep without the Godep pathing.

@jdorfman
Copy link
Member

jdorfman commented Aug 3, 2015

looks good, @jmervine merge and push package whenever

@jmervine
Copy link
Contributor Author

jmervine commented Aug 6, 2015

@jdorfman I'm merging this now. Keep in mind there's some minor breaking changes. If support gets any pings on it, point them to v1 via import "gopkg.in/jmervine/go-maxcdn.v1" or to to read the updated docs.

jmervine added a commit that referenced this pull request Aug 6, 2015
Merging creack/cleanup - v2
@jmervine jmervine merged commit 8c616f0 into master Aug 6, 2015
@jmervine jmervine deleted the jmervine-v2 branch August 6, 2015 06:29
@jdorfman
Copy link
Member

jdorfman commented Aug 6, 2015

got it thanks @jmervine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants