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

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

Closed
AlekSi opened this issue Dec 19, 2018 · 6 comments
Closed

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

AlekSi opened this issue Dec 19, 2018 · 6 comments

Comments

@AlekSi
Copy link
Member

AlekSi commented Dec 19, 2018

No description provided.

@quasilyte
Copy link
Contributor

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.

@quasilyte
Copy link
Contributor

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.

@AlekSi
Copy link
Member Author

AlekSi commented Dec 21, 2018

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.

@AlekSi
Copy link
Member Author

AlekSi commented Aug 31, 2020

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

@AlekSi AlekSi closed this as completed Aug 31, 2020
@quasilyte
Copy link
Contributor

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

@AlekSi
Copy link
Member Author

AlekSi commented Aug 31, 2020

😿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants