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

fix issue #79: add /v6 module import suffix #80

Merged
merged 1 commit into from
May 15, 2020

Conversation

mwf
Copy link
Contributor

@mwf mwf commented May 7, 2020

@sohamkamani I've added /v6 import suffix to resolve issue #79 introduced with v5.1.0 release with #40.

According to https://github.com/golang/go/wiki/Modules#why-must-major-version-numbers-appear-in-import-paths the major version must present in import paths.

If you wish, I could do it in v5 module, to fix the buggy v5.1.0 version, but it's strongly recommended to increase major version after adopting go modules: https://github.com/golang/go/wiki/Modules#incrementing-the-major-version-when-first-adopting-modules-with-v2-packages, so I've made it v6.

And please have a look at https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher
In short, there could be some problems with godep users with major go.mod in master. So we could introduce better compatibility with v6 subdirectory for non-gomod users.
But if you dropped Gopkg.toml support, maybe it's unnecessary.

Let's discuss an fix it ASAP to have better go.mod support 🙇

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4ba367d on mwf:fix-gomod-module into 1aedfb0 on gojek:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4ba367d on mwf:fix-gomod-module into 1aedfb0 on gojek:master.

@rShetty rShetty merged commit a006dd7 into gojek:master May 15, 2020
@rShetty
Copy link
Contributor

rShetty commented May 15, 2020

@mwf Thanks for the PR. This makes sense. Merging this.

@rShetty
Copy link
Contributor

rShetty commented May 15, 2020

@mwf Can you also help change the README to import the package with the new changes?

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.

4 participants