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

Remove Go module #41

Merged
merged 2 commits into from
Nov 1, 2018
Merged

Remove Go module #41

merged 2 commits into from
Nov 1, 2018

Conversation

aofei
Copy link
Contributor

@aofei aofei commented Oct 24, 2018

I don't think the current version 2 of this project can support the Go module (see #40). So I revert my previous changes.

@tdewolff
Copy link
Owner

Hi @aofei I think we can support Go modules, but I don't think we need the v2 path. Since v0 and v1 are not supported by Go modules, it was my understanding that we could leave the paths be. See the second comment at https://www.reddit.com/r/golang/comments/98h2yb/introduction_to_go_modules/e4hmvgq/?context=3

@aofei
Copy link
Contributor Author

aofei commented Oct 25, 2018

I'm a bit confused now. 😩

According to the Semantic Import Versioning, I think the /v2 suffix is mandatory, not optional.

...
As a result of semantic import versioning, code opting in to Go modules must comply with these rules:

  • Follow semantic versioning (with tags such as v1.2.3).
  • If the module is version v2 or higher, the major version of the module must be included in both the module path in the go.mod file (e.g., module example.com/my/mod/v2) and the package import path (e.g., import "example.com/my/mod/v2/foo").
  • If the module is version v0 or v1, do not include the major version in either the module path or the import path.

...

BTW, I think the russross/blackfriday has already experienced this. But their final solution is similar to what you share. See russross/blackfriday#487 and russross/blackfriday#488.

@aofei
Copy link
Contributor Author

aofei commented Oct 25, 2018

Ah, I think I understand their workaround. Their master branch is only for v1, and v2 is placed separately in a new branch and the /v2 suffix is added to the go.mod file. Then they released v1.5.2 and v2.0.1 respectively. See https://github.com/russross/blackfriday/blob/master/go.mod and https://github.com/russross/blackfriday/blob/v2/go.mod.

@aofei
Copy link
Contributor Author

aofei commented Oct 25, 2018

First, create a v1 branch and release v1.1.1.

Next, add the /v2 suffix to the go.mod file.

Finally, create a v2 branch and release v2.3.5.

By doing this, we can strictly follow the Semantic Import Versioning.

What do you think? @tdewolff

@aofei
Copy link
Contributor Author

aofei commented Oct 25, 2018

And, if we do that, I think the v1 branch should start with commit 0334a86.

@aofei
Copy link
Contributor Author

aofei commented Oct 26, 2018

@tdewolff

Since you just released tdewolff/minify v2.3.6, I think this problem must be solved ASAP.

See this:

minify-failed-go-module

@tdewolff
Copy link
Owner

tdewolff commented Nov 1, 2018

Sigh...see golang/go#25967 at the bottom. Basically a new major version needs to be released, which I don't really feel like doing to be honest. I guess I'll keep minify at v0.0.0-HASH for now...

EDIT: looks like v.2.3.4+incompatible works indeed!

EDIT2: Never mind. After releasing v2.3.7 for minify (and v2.3.4 for parse) to use Go modules, it now automatically picks an older version for minify (v2.3.6) and parse (v2.3.3) for some reason. What a mess! Do I really have to change all import paths and release major version 3!?

EDIT3: setting the module name in go.mod to be module github.com/tdewolff/parse/v2 seems to have the effect of adding a dependency for github.com/tdewolff/parse v2.3.3+incompatible...

@tdewolff tdewolff merged commit 3b56c4a into tdewolff:master Nov 1, 2018
@aofei aofei mentioned this pull request Nov 2, 2018
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.

2 participants