x/tools/go/packages: clarify behavior when given zero patterns #28767
Comments
mvdan
added
the
NeedsDecision
|
From the
By that light, I think that it should treat no packages as the same as |
|
I agree there is a problem here. The tricky thing is that the underlying build systems vary in their treatment of an empty argument list. The go tool treats [ ] as ["."], but Blaze and Bazel treat [ ] as an empty request: a no-op. Furthermore "." is not a valid argument to Bazel; the closest concept is ":all", but users would not conventionally expect [ ] to mean [":all"]. If we follow the principle outlined in the docs ("the conventions of the underlying build system") then the interpretation of [ ] may vary, which is fine for interactive command-line users who know the conventions of their build system. If an application driving packages.Load needs a way to say "all the packages in the current directory", then perhaps we should add another special query (like name=..., file=...), but I have not yet come across the need. |
|
I imagined that this behavior might only concern the In that case, I presume we can simply change the driver's behavior, and leave the docs untouched. |
|
I think we should leave this up to the driver but call out explicitly in the docs to Load that an empty pattern list is driver defined behavior, because it might surprise some people, and programmatic uses that are trying to avoid driver defined behavior need to know to avoid this case. |
|
Would a patch be welcome for this? It would likely take me less time to send a fix to x/tools than to apply a workaround in a handful of tools, to then undo it later :) |
|
Yes, absolutely, we love patches :) |
|
Ah cool. I wasn't sure, since the issue had been assigned. |
|
I hereby gift this issue to you! Thanks!! |
|
Christmas came early :) |
|
This was fixed by https://go-review.googlesource.com/c/tools/+/155898 |
|
Agh, I keep referencing issues wrong in the subrepo CLs. |
I've ported a few tools over from a mix of https://github.com/kisielk/gotool and
x/tools/go/loaderover togo/packages. The transition was quite simple for most tools, and everything still worked fine.Until today, when I happened to notice that running
unparamwas much faster thanunparam .. The reason is that, of course, the former does nothing.The
gotoolpackage was extracted fromcmd/go, so it automatically treated zero patterns as".": kisielk/gotool@49706c2I presume it's fair enough if
go/packagesgoes a different direction, but I think that something should be done to avoid confusion. I'm willing to bet that I'm not the only one that has ported a Go tool over togo/packagesand not noticed that the zero argument case has silently broken.Options that come to mind, in order of personal preference:
It may return an empty list of packages without an error, for instance for an empty expansion of a valid wildcardto also include the zero patterns case.Load(cfg)behave likeLoad(cfg, ".")packages.Loaderror when given zero patternsIf we go with the doc fix route, I'd suggest including the string
"."somewhere in the docs. That's the main string I was using to search for this edge case's behavior./cc @ianthehat @matloob @alandonovan
The text was updated successfully, but these errors were encountered: