golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.87k stars 17.52k forks source link

io: add ReadSeekCloser interface #40962

Closed mohamedattahri closed 3 years ago

mohamedattahri commented 4 years ago

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.

ianlancetaylor commented 4 years ago

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

mohamedattahri commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

davecheney commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Seems worth adding to me.

mohamedattahri commented 3 years ago

Don't see any downside.

rsc commented 3 years ago

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

mohamedattahri commented 3 years ago

Implementation should be trivial, but happy to contribute.

ianlancetaylor commented 3 years ago

@mohamedattahri Go for it.

gopherbot commented 3 years ago

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

rsc commented 3 years ago

No change in consensus, so accepted.

mohamedattahri commented 3 years ago

@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 commented 3 years ago

@mohamedattahri Thanks for implementing it.