-
Notifications
You must be signed in to change notification settings - Fork 79
all: consider using golang.org/x/{mod,tools} packages and removing duplicates #74
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
Comments
Thanks for the heads up (FYI we've been tracking this for a while, since @rogpeppe raised golang/go#28101). The one thing we'll need to keep in mind is that we've already released v1 so we can't make any breaking changes. |
I have a question about that. This repository is a copy of internal packages which may have breaking changes over time. Are you not planning to make breaking changes here if/when that happens? Or do you mean you can't make breaking changes in v1, but can in v2, etc.? |
@dmitshur yes, the idea of this repo is that it provides a stable API even if upstream changes incompatibly, if possible. |
FWIW, this is becoming less of a problem now that more packages have been added to x/mod. It's possible to switch to using x/mod atomically and the packages there are naturally compatible with each other. For example, see shurcooL/home@dce9a6e. That means the value of doing the work to resolve this issue is lower now. (Thank you for providing those packages before they were added to x/mod; it has been very helpful.) |
x/tools is also slowly gaining what's been here for years; https://godoc.org/golang.org/x/tools/txtar is an example. Should we retitle this issue to be about any packages now provided by the x/... modules? |
One thought that came to mind: We could replace the txtar or module packages with their It would be nice at a high level, because then a downstream user of all of these modules would see less code duplication, and it would also be easier for us in the long term - less code to maintain. However, at a small scale it's also unfortunate to add more module dependencies. Any user of testscript would suddenly now pull I still lean in favor of doing that though, given how the |
I took a look at txtar, and it's worth noting that it's not a plain duplicate. Even though the APIs provided by https://pkg.go.dev/golang.org/x/tools/txtar are compatible with https://pkg.go.dev/github.com/rogpeppe/go-internal/txtar, note that our fork has more features, like Quote and Write. I think it only really makes sense to deduplicate if we can properly deprecate our package. "use upstream txtar for parsing but our package for writing or quoting" isn't a good strategy. So I think our best bet is to suggest these features upstream, and then turn our txtar package into a deprecated one that simply forwards the APIs. |
@rogpeppe and I did this in PRs like:
The go-internal module should now be quite a bit smaller and easier to maintain. If you spot more packages which are good candidates for this treatment, feel free to raise a new issue or open a PR. For now, I think we can mark this issue as resolved. |
Uh oh!
There was an error while loading. Please reload this page.
Now that golang/go#31761 has been accepted and golang.org/x/mod is starting to get some of the same packages, I think it would make sense to start using them in this repository (and potentially removing duplicates).
For example,
golang.org/x/mod/module
now exists and defines themodule.Version
type. If someone uses that package in their project, the problem is that the types used ingb.xjqchip.workers.dev/rogpeppe/go-internal/modfile
are not compatible and need to be converted.To make things better,
github.com/rogpeppe/go-internal/modfile
can be updated to usegolang.org/x/mod/module
directly andgb.xjqchip.workers.dev/rogpeppe/go-internal/module
can be removed.Alternatively,
github.com/rogpeppe/go-internal/module
can be updated to defineVersion
as a type alias ofgolang.org/x/mod/module.Version
.In my personal opinion, it would be most helpful if this repository contained only the bare minimum packages that are not available elsewhere, but that's a decision for the project owner(s).
The text was updated successfully, but these errors were encountered: