rubensayshi commented 3 years ago

add support for go modules by

using patsy.Dirs instead of recursively iterating through patsy.Dir calls.
updating the tools/go/packages package.

improved error reporting a bit by adding and printing stack traces.

depends on https://github.com/dave/patsy/pull/1 being merged and updating the go.mod in this PR to the latest version, until then it's been set to use my branch/fork of patsy.

rubensayshi commented 3 years ago

I've tried making tests with with a mix of GOPATH and go modules, but being mutually exclusive it's becoming a mess, needs some work to just create proper tests for both.

or drop support for GOPATH entirely ... @dave ?

imsodin commented 3 years ago

Very nice !
As in: Works for me on syncthing in module mode.

until then can easily be tested by checking out the patsy PR locally and adding replace github.com/dave/patsy => /path/to/local/checkout/of/patsy to the go.mod file of this repo.

To avoid having to check out patsy one can use

replace github.com/dave/patsy latest => github.com/rubensayshi/patsy courtney-gomod-prep

Cutting a v2 module and using that for go modules only would be a simple option to keep backwards compat if that's desired.

rubensayshi commented 3 years ago

I'm gonna discuss both the courtney and patsy changes here, I think that makes more sense than discussing the patsy changes in the other PR because then they lack context...

went back over everything and made a cleaner separation between how the patsy.Builder works for gomod and gopath mode, as a result everything works well in both modes now.

in patsy I use go list -f '{{.ImportPath}}:{{.Dir}}' for almost everything now, which helps a lot in making everything work well!

I think the only thing which I've broken is that go list doesn't work if there's no .go files in a package, I've retained the fallback code in patsy in gopath mode, but there's no good equivalent in go modules mode.
I don't see any reason why this is necessary though?

dave commented 3 years ago

Hey I see this and it's totally awesome that you're working on this stuff! Right now I literally don't have 5 minutes to sit down and look at it but I'll try to make some quality time to refamiliarise myself with the project and take a good look... some time in the next couple of weeks. Sorry I can't be more reactive right now...

rubensayshi commented 3 years ago

@dave no worries, just appreciate that you actually took a brief moment to let me know !

rubensayshi commented 3 years ago

hey @dave this is a friendly reminder that this is still open, if you're still swamped then ignore it ;-)

dave commented 3 years ago

Hmmm I'm not really working on Go stuff right now, so unlikely I'll ever get around to testing this. If I get someone else to do a full test I'd be happy to merge it blindly... @imsodin have you tested this change enough for me to merge?

imsodin commented 3 years ago

Just checked again: Still works as in can run the tests and create coverage. I didn't do any in-depth testing or review.

If you're not working on this any more, maybe it it's an option to add @rubensayshi as a contributor and/or move the repo into a to-be-created org with an "open-governance" model? I had considered contributing at some point but didn't have the time to port to go modules first, so maybe after this I'll do it - even though I don't remember right now what it was :)

dave commented 3 years ago

Good idea. I just added @rubensayshi as a contributor of courtney and patsy. Feel free to merge if you think it's ready.

rubensayshi commented 3 years ago

been trying it out on our own repos and it's working well, I think it's ready to merge.

and I got time to deal with any issues that might arrise

codecov-commenter commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@9bc8a11). Click here to learn what that means. The diff coverage is 100.00%.

:exclamation: Current head 6723249 differs from pull request most recent head 2d687a7. Consider uploading reports for the commit 2d687a7 to get more accurate results

@@            Coverage Diff             @@
##             master       #21   +/-   ##
==========================================
  Coverage          ?   100.00%           
==========================================
  Files             ?         6           
  Lines             ?       406           
  Branches          ?         0           
==========================================
  Hits              ?       406           
  Misses            ?         0           
  Partials          ?         0

Impacted Files	Coverage Δ
tester/tester.go	`100.00% <ø> (ø)`
courtney.go	`100.00% <100.00%> (ø)`
scanner/scanner.go	`100.00% <100.00%> (ø)`
shared/shared.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9bc8a11...2d687a7. Read the comment docs.

dave / courtney

add support for go modules #21

Codecov Report