Add back support for go modules (fixes #155) #159
Conversation
|
Looks like CI hates it.. I don't have time to dig into it today but I'll try to look into it this week. If anyone sees something obvious let me know. |
|
I checked out the branch and installed the binary, and tested it against github.com/uber/prototool, checked out outside of my |
|
Thanks for the patch. I can merge it once the tests are fixed |
|
Sounds good - I'll see if I can figure out what's up with the tests tomorrow. |
coopernurse
force-pushed
the
155-go-modules
branch
from
2c5d947
to
4c22418
Compare
|
Well, the good news is CI passes after I rebased. The bad news is that Go 1.11 modules don't seem to work anymore. Hmm.. Looking into it. |
|
Nevermind - I didn't have |
|
Just tested again against 4c22418 (last commit on the 155-go-modules branch), this works on github.com/uber/prototool. |
| loadcfg.ParserMode = parser.ParseComments | ||
| } | ||
| // loadPackages is used for testing. | ||
| var loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { |
There was a problem hiding this comment.
Can't you just do var loadPackages = packages.Load since the function signature is the same? And if so, is this variable needed?
There was a problem hiding this comment.
The variable is replaced by a different function in the unit tests. The definition could probably be simplified down as you propose.
| cfg := &packages.Config{ | ||
| Mode: packages.LoadAllSyntax, | ||
| Tests: !c.WithoutTests, | ||
| BuildFlags: []string{fmt.Sprintf("-tags=%s", strings.Join(c.Tags, " "))}, |
There was a problem hiding this comment.
Does this actually work with multiple tags? I feel like it would need to be quoted. Is this even needed anymore?
There was a problem hiding this comment.
We definitely should continue to support build tags, I don't know if this works or not as there are no tests for the functionality.
There was a problem hiding this comment.
Quoting is for shells, not argv. This should work, but I didn't test the PR.
There was a problem hiding this comment.
This produces a single string though, not a set of strings (which would make sense to do)
There was a problem hiding this comment.
BuildFlags is an array of arguments to pass to the go tool, in addition to the flags go/packages generates. Same semantics as os.Args, exec.Command or execve.
go list -tags="foo bar" becomes
([]string) (len=3 cap=3) {
(string) (len=5) "go",
(string) (len=4) "list",
(string) (len=13) "-tags=foo bar"
}
There was a problem hiding this comment.
I've put up a PR that adds tests for this at #160.
I applied the test to this PR (with the necessary modifications: see coopernurse#1) and verified that this seems to work as expected.
There was a problem hiding this comment.
Fabulous. I merged your tests in and they're now included in this PR (which was rebased off master today).
…with-gopackages"" This reverts commit 1787c4b.
coopernurse
force-pushed
the
155-go-modules
branch
from
5b0b0b9
to
6bfc115
Compare
|
Anything left to do here? :-) This still works from my testing. |
|
I think we're all set unless any maintainers / reviewers have additional thoughts. If not let's merge it! |
|
Merged! |
|
Just wanted to add a thanks for everyone who had input and worked on this. Working on Go projects isn't my full-time job any more and my available hours for maintaining my open source packages are fairly limited (plus I was on vacation the last few weeks). I really appreciate the support of all the community members in helping out with this package. |
|
@kisielk thanks for merging! Could we also get a tagged release for this? I don't believe this change broke anything (except possibly not working for some older versions of Go), so I think v1.2.0 should be good. |
|
I've tagged v1.2.0 now. |
This work was done by @domgreen (see: https://github.com/domgreen/errcheck)
I've only tested this with Go 1.11 so I'm not sure if older go versions will still work properly.