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

Ensure org name in the import path is always NYTimes #76

Merged
merged 4 commits into from
Feb 11, 2019
Merged

Ensure org name in the import path is always NYTimes #76

merged 4 commits into from
Feb 11, 2019

Conversation

fsouza
Copy link
Contributor

@fsouza fsouza commented Feb 9, 2019

Fun times are coming.

Copy link

@marwan-at-work marwan-at-work left a comment

Choose a reason for hiding this comment

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

one comment, but LGTM

@@ -1,4 +1,4 @@
package gziphandler
package gziphandler // import "github.com/NYTimes/gziphandler"
Copy link

@marwan-at-work marwan-at-work Feb 11, 2019

Choose a reason for hiding this comment

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

do we need this vanity import? Pretty sure I think Go Modules ignores it and the old Go will freak out anyway if a user did lower case "get"

Copy link
Contributor Author

@fsouza fsouza Feb 11, 2019

Choose a reason for hiding this comment

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

@marwan-at-work because gziphandler doesn't have sub-packages, there's nothing enforcing what case is used in the import path.

Before this change:

% docker run golang go get -v github.com/nytimes/gziphandler
github.com/nytimes/gziphandler (download)
github.com/nytimes/gziphandler
% docker run golang go get -v github.com/NYTimes/gziphandler
github.com/NYTimes/gziphandler (download)
github.com/NYTimes/gziphandler

Using the CASE-fold org, where the canonical import path is declared:

% docker run golang go get -v github.com/CASE-fold/gziphandler
github.com/CASE-fold/gziphandler (download)
package github.com/CASE-fold/gziphandler: code in directory /go/src/github.com/CASE-fold/gziphandler expects import "github.com/case-fold/gziphandler"
% docker run golang go get -v github.com/case-fold/gziphandler
github.com/case-fold/gziphandler (download)
github.com/case-fold/gziphandler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(same thing applies to dep)

Choose a reason for hiding this comment

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

Oh I see, the lack of sub packages makes the import path work both ways...maybe that's a good thing 😄

Copy link
Contributor Author

@fsouza fsouza Feb 11, 2019

Choose a reason for hiding this comment

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

Not really, because every package in a build have to agree on the import path, so if one of your dependencies wants to use nytimes while others want to use NYTimes, you're busted.

This is what happened to logrus: while it does have sub-packages, the package github.com/sirupsen/logrus doesn't depend on any of the them. The internet crashed once some packages started using sirupsen while others were still using Sirupsen.

Choose a reason for hiding this comment

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

very good point ^

Copy link
Contributor

@jprobinson jprobinson left a comment

Choose a reason for hiding this comment

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

🌮

@fsouza fsouza merged commit ae3e73a into nytimes:master Feb 11, 2019
@fsouza fsouza deleted the prep-for-rename branch February 11, 2019 20:03
fsouza pushed a commit to nytimes/logrotate that referenced this pull request Feb 12, 2019
Same as nytimes/gziphandler#76.

Also giving the travis.yml file some love.
fsouza pushed a commit to nytimes/logrotate that referenced this pull request Feb 12, 2019
Same as nytimes/gziphandler#76.

Also giving the travis.yml file some love.
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.

3 participants