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

x/tools/go/packages: NeedExportsFile and Package.ExportFile are inconsistent #36302

Closed
mvdan opened this issue Dec 29, 2019 · 6 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Dec 29, 2019

The former uses the plural Exports, and the latter uses the singular Export.

I think this was a typo made in https://golang.org/cl/162140 that never got spotted, unfortunately.

I see multiple options, where 1 is the one I'd prefer the most:

  1. Add the NeedExportFile const as a duplicate of NeedExportsFile, and deprecate the latter (assuming that "export file" is the right term)
  2. Leave both names as-is, and fix the NeedExportsFile doc to say that it adds ExportFile, not ExportsFile
  3. Add the ExportsFile field as a duplicate of ExportFile, and deprecate the latter (assuming that "exports file" is the right term)

/cc @matloob @stamblerre @ianthehat @heschik

@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository. labels Dec 29, 2019
@mvdan mvdan added this to the Unreleased milestone Dec 29, 2019
@mvdan
Copy link
Member Author

mvdan commented Dec 29, 2019

The reason I think the singular is correct is because in many places in the official docs, one can find "export data". So I think "export file" is just short for "export data file".

Unless you get into the "data is plural" argument, making "exports" short for "export data"... Then I'm not sure :)

@matloob
Copy link
Contributor

matloob commented Dec 30, 2019

Ugh, I'm ashamed of this.

I'm leaning towards option 2, as much as it's bad.
Option 1 is okay, but it means that we'll have two names around forever. Even if it's deprecated, it will always be in the godoc, and code, and it's just a little bit more confusion.
Option 3 means that we'd have to fill out both fields forever, which isn't optimal

@mvdan
Copy link
Member Author

mvdan commented Dec 31, 2019

Hm, I think 1 is best as we could just teach godoc (or the package discovery site?) to more actively hide deprecated APIs, particularly those that have a direct replacement.

Also, I realise there is no plan for semver releases of x/tools right now, but I imagine a not so distant future where we might have a go/packages v1 release as part of some module, at which point these API quirks could potentially be cleaned up. If we decide to be extra conservative right now, I think we should at least leave a TODO around.

@matloob
Copy link
Contributor

matloob commented Jan 2, 2020

Okay, let's add a TODO. I'm already dissatisfied having the deprecated LoadModes. I'd be more comfortable going with Option 1 once Godoc and the discovery site better hide deprecated APIs, or even once there's a plan for a v1...

@mvdan
Copy link
Member Author

mvdan commented Jan 3, 2020

SGTM. We should fix the godoc too though, as the current one points the user to a field name that doesn't exist.

@gopherbot
Copy link

Change https://golang.org/cl/215980 mentions this issue: go/packages: fix doc for NeedExportsFile

@golang golang locked and limited conversation to collaborators Jan 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

3 participants