go-internal icon indicating copy to clipboard operation
go-internal copied to clipboard

all: consider using golang.org/x/{mod,tools} packages and removing duplicates

Open dmitshur opened this issue 5 years ago • 7 comments

Now that https://github.com/golang/go/issues/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 the module.Version type. If someone uses that package in their project, the problem is that the types used in github.com/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 use golang.org/x/mod/module directly and github.com/rogpeppe/go-internal/module can be removed.

Alternatively, github.com/rogpeppe/go-internal/module can be updated to define Version as a type alias of golang.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).

dmitshur avatar Jun 29 '19 17:06 dmitshur

Thanks for the heads up (FYI we've been tracking this for a while, since @rogpeppe raised https://github.com/golang/go/issues/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.

myitcv avatar Jun 29 '19 20:06 myitcv

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 avatar Jul 01 '19 01:07 dmitshur

@dmitshur yes, the idea of this repo is that it provides a stable API even if upstream changes incompatibly, if possible. If not, we'll just move to v2.

rogpeppe avatar Jul 01 '19 22:07 rogpeppe

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 https://github.com/shurcooL/home/commit/dce9a6ea048c9ad7a398c1241643d17ec9436c6c. 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.)

dmitshur avatar Nov 04 '19 00:11 dmitshur

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?

mvdan avatar Oct 31 '20 13:10 mvdan

One thought that came to mind: We could replace the txtar or module packages with their x/... counterparts by adding module dependencies and forwarding the API calls without breaking users.

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 x/tools as well - which is not the end of the world, but is objectively more code to pull.

I still lean in favor of doing that though, given how the x/tools and x/mod modules are special already - they are vendored into the standard library. So using them in go-internal feels natural.

mvdan avatar Jun 14 '22 10:06 mvdan

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.

mvdan avatar Jul 28 '22 21:07 mvdan

@rogpeppe and I did this in PRs like:

  • https://github.com/rogpeppe/go-internal/pull/208
  • https://github.com/rogpeppe/go-internal/pull/209
  • https://github.com/rogpeppe/go-internal/pull/210
  • https://github.com/rogpeppe/go-internal/pull/211
  • https://github.com/rogpeppe/go-internal/pull/213

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.

mvdan avatar May 08 '23 10:05 mvdan