go-reform/reform

Migrate to golang.org/x/tools/go/packages

AlekSi opened this issue · 6 comments

Migrate to golang.org/x/tools/go/packages

Note that taking the result of packages.Load might be non-trivial, since it can return arbitrary number of packages for a given "pattern". And even for simple targets like canonical import path (say, foo/bar) it can return from 1 to 4 entries (4 for a package with both tests and external tests).

Depending on how you use it, that might lead to a duplicated objects.
Some linters de-duplicate their output due to that (seems like a rough solution to me).
The other way is to handle 1-N possible package sets properly without visiting already marked entities.

I can take a look how and what reform uses in order to estimate how painless it would be to port it to the new fancy stuff.

Looks like only go/parser is used, without go/types.
https://github.com/go-reform/reform/blob/v1-stable/parse/file.go#L117

If only file-based patterns are required, I don't see a big win from using packages here, unless I'm missing something. packages were meant to solve a problem when it's hard to load a package given its name or import path depending on the environment (with or without modules), it also makes it easier to load tests+external tests along the target package.

reform tool can accept directories or package paths:

reform/reform/main.go

Lines 144 to 160 in 8e91a79

// process arguments
for _, arg := range flag.Args() {
// import arg as directory or package path
var pack *build.Package
s, err := os.Stat(arg)
if err == nil && s.IsDir() {
pack, err = build.ImportDir(arg, 0)
}
if os.IsNotExist(err) {
err = nil
}
if pack == nil && err == nil {
pack, err = build.Import(arg, wd, 0)
}
if err != nil {
logger.Fatalf("%s: %s", arg, err)
}

Maybe we should deprecate and then remove the ability to invoke reform with a package path. It will accept only directories. Packages can be passed via go generate. The only downside I see is that spawning a new process for each model file (that's how go generate works) is slow on Windows.

Maybe we should do nothing – go/build has code to invoke go tool when modules are enabled — that way we also not need an extra dependency.

Ok, let's do nothing, go/build is good enough

It's 2020 and go/packages is still painful to use.

😿