Navigation Menu

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

io: add ReadSeekCloser interface #40962

Closed
mohamedattahri opened this issue Aug 21, 2020 · 16 comments
Closed

io: add ReadSeekCloser interface #40962

mohamedattahri opened this issue Aug 21, 2020 · 16 comments

Comments

@mohamedattahri
Copy link
Contributor

Background

I have several projects where I find myself needing to implement an interface that combines io.ReadSeeker with io.Closer.

Quick search on Github reveals that it's not an uncommon need. In some instances, the interface is implemented more than once in the same project to avoid unnecessary imports of some packages.

Description

Proposal is to simply add a new ReadSeekCloser interface to the io package.

type ReadSeekCloser interface {
    io.ReadSeeker
    io.Closer
}

Costs

Can't think of anything other than the addition to the io package.

@gopherbot gopherbot added this to the Proposal milestone Aug 21, 2020
@ianlancetaylor ianlancetaylor changed the title Proposal: io: Add ReadSeekCloser interface proposal: io: add ReadSeekCloser interface Aug 21, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 21, 2020
@ianlancetaylor
Copy link
Contributor

How do we draw the line between interfaces to add to the standard library and interfaces to leave out?

@mohamedattahri
Copy link
Contributor Author

We obviously don't want to have to expose every interface combination possible in the standard library, so my first intuition is to default to no, resist establishing rules/criteria, and adopt a case-by-case approach.

A strong argument in favor of this approach is that not all packages are the same. Specifically, the purpose of the io package is to expose a collection of interfaces designed to unify the Go ecosystem around the same building blocks. Consequently, adding more interfaces to it seems more constructive than not.

In other words, adding ReadSeekCloser to the io package can only be positive, because it would:

  1. Unify the ecosystem around a common definition of seemingly popular type;
  2. Bring consistency because it's the last missing combination of the single-function interfaces it exposes.

@davecheney
Copy link
Contributor

Unify the ecosystem around a common definition of seemingly popular type

But this is not true. In go, interface equality isn’t defined by the methods of the interface, not its name. Thus anyone can declare the compound interface you outline above and it will operate identically.

@mohamedattahri
Copy link
Contributor Author

Absolutely, but that doesn't mean the ecosystem wouldn't benefit from a single declaration of the type. Otherwise, why have io.Readeror io.ReadWriteSeeker, when every package could define their own? Seems unnecessary in light of the low cost to add it.

So the two most important factors here in my opinion are:

  • Demand: how many times have people had to define it?
  • Import cost: can we naturally add it to a package that's already part of every project, or is gonna cost an extra import?

@davecheney
Copy link
Contributor

Otherwise, why have io.Readeror io.ReadWriteSeeker

I’ve often asked myself this question, but I don’t think the answer is that the existing interface declarations in the io package represent a precedent for more interface declarations in the io package.

@rsc
Copy link
Contributor

rsc commented Sep 23, 2020

I started looking at this and it does get a defined a lot. Will report back with stats but it might be worth doing.

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 23, 2020
@rsc
Copy link
Contributor

rsc commented Sep 30, 2020

After removing duplicates due to rampant GitHub forking, I'm left with an interface named Read(er)?Seek(er)?Close(r)? being defined in all the modules in the list below, which seems like enough. A small number of them are not defined as Read+Seek+Close. Maybe if we'd defined io.ReadSeekCloser from the start, those would have picked a clearer name.

aahframework.org/vfs.v0
agola.io/agola
badc0de.net/pkg/go-tibia
github.com/ipfs/go-ipfs
github.com/Azure/azure-sdk-for-go/sdk/azcore
github.com/BlackDuck888/storj
github.com/EngoEngine/audio
github.com/EngoEngine/engo
github.com/EngoEngine/systems
github.com/IBM/ibmcloud-cos-cli
github.com/JaSei/pathutil-go
github.com/Microsoft/hdfs-mount
github.com/NGnius/internet-of-music/server
github.com/andreimarcu/linx-server
github.com/andrew-torda/goutil
github.com/apcera/util
github.com/applike/gosoline
github.com/aptly-dev/aptly
github.com/balena-os/balena-engine
github.com/brentp/go-athenaeum
github.com/cloudfoundry/bosh-cli
github.com/compozis/storage
github.com/couchbaselabs/cbfs
github.com/denisbetsi/pdfcpu
github.com/docker/distribution
github.com/dustin/go-wikiparse
github.com/ellotheth/pipethis
github.com/engoengine/engo
github.com/eriq-augustine/elfs
github.com/euank/go-kmsg-parser
github.com/form3tech-oss/terraform-provider-form3
github.com/garyburd/s3web
github.com/garyburd/staticsite
github.com/git-lfs/git-lfs
github.com/go-git/go-git
github.com/gocaveman/caveman
github.com/gofiber/compress
github.com/gohugoio/hugo
github.com/gonutz/payload
github.com/google/fleetspeak
github.com/gravitational/gravity
github.com/gravitational/roundtrip
github.com/hajimehoshi/ebiten
github.com/hawser/git-hawser
github.com/ihexxa/quickshare
github.com/ipfs/go-unixfs
github.com/ipfs/interface-go-ipfs-core
github.com/juju/charmstore
github.com/juju/juju
github.com/klauspost/compress
github.com/klauspost/readahead
github.com/kopia/kopia
github.com/linlexing/recfile
github.com/lox/httpcache
github.com/nightlyone/checklogfile
github.com/nimezhu/netio
github.com/opctl/opctl/sdks/go
github.com/oracle/smith
github.com/pdfcpu/pdfcpu
github.com/rainycape/gondola
github.com/raspi/heksa
github.com/rclone/rclone
github.com/rstorlabs/stash
github.com/saracen/go7z-fixtures
github.com/shipwire/swutil
github.com/shumon84/mogit
github.com/sourcegraph/ctxvfs
github.com/taskcluster/taskcluster-worker
github.com/tgulacsi/go
github.com/tinode/chat
github.com/traPtitech/traQ
github.com/trumanw/negroni-cache
github.com/uber/aresdb
github.com/uber/aresdb
github.com/ungerik/go-fs
github.com/wal-g/wal-g
github.com/xmidt-org/webpa-common
github.com/zikichombo/codec
go4.org
golang.org/x/tools
storj.io/storj
www.velocidex.com/golang/velociraptor

The full definitions are in the attached readseekclose.txt. Remember that this drops duplicates due to forks, and also duplicate definitions of ReadSeekCloser within a single module (in different packages).

@rsc
Copy link
Contributor

rsc commented Sep 30, 2020

Seems worth adding to me.

@mohamedattahri
Copy link
Contributor Author

Don't see any downside.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Oct 7, 2020
@rsc
Copy link
Contributor

rsc commented Oct 7, 2020

Based on the discussion above, this seems like a likely accept.

@mohamedattahri
Copy link
Contributor Author

Implementation should be trivial, but happy to contribute.

@ianlancetaylor
Copy link
Contributor

@mohamedattahri Go for it.

mohamedattahri added a commit to mohamedattahri/go that referenced this issue Oct 12, 2020
Research showed that this interface is defined
frequently enough in real-world usage to justify
its addition to the standard library.

Related to golang#40962
@gopherbot
Copy link

Change https://golang.org/cl/261577 mentions this issue: io: add a new ReadSeekCloser interface

mohamedattahri added a commit to mohamedattahri/go that referenced this issue Oct 13, 2020
Research showed that this interface is defined frequently enough in real-world usage to justify its addition to the standard library.

Fixes golang#40962
@rsc
Copy link
Contributor

rsc commented Oct 14, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Oct 14, 2020
@mohamedattahri
Copy link
Contributor Author

@rsc @ianlancetaylor – This was my first contribution. Quick note to thank you both and congratulate the team for a super smooth process and no learning curve for any lambda Go developer using Github. Cheers.

@ianlancetaylor
Copy link
Contributor

@mohamedattahri Thanks for implementing it.

@rsc rsc removed this from the Proposal milestone Oct 17, 2020
@rsc rsc added this to the Go1.16 milestone Oct 17, 2020
@rsc rsc changed the title proposal: io: add ReadSeekCloser interface io: add ReadSeekCloser interface Oct 17, 2020
@golang golang locked and limited conversation to collaborators Oct 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants