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: re-add SizedReaderAt #15822

Closed
nightlyone opened this issue May 24, 2016 · 6 comments
Closed

io: re-add SizedReaderAt #15822

nightlyone opened this issue May 24, 2016 · 6 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@nightlyone
Copy link
Contributor

io.ReadSizedAt has been added after 3 years of discussion and removed within 3h in #15818

Let's re-evaluate it's usefulness and design here after Go-1.7 shipped.

Relates to #14889 and #7263

@nightlyone
Copy link
Contributor Author

@rsc Could you tag this as Unplanned or Go1.8 as needed?

@rsc rsc added this to the Go1.8 milestone May 24, 2016
@rsc
Copy link
Contributor

rsc commented May 24, 2016

Added to Go 1.8, thanks. I certainly intended that we would discuss after Go 1.7.
Where are the 3 years of past discussion, though?

@extemporalgenome
Copy link
Contributor

Size() int64 would be the first method mentioned in package io which does not return an error -- that does seem quite inconsistent.

As far as I can tell, Sizer's original use came from os.FileInfo. Subsequent cases skipped the Stat() (FileInfo, error) indirection.

A few options I see (in decreasing order of personal preference):

Function that produces a conventional SizeReaderAt from any StatReaderAt

We could provide a function like:

func NewSizeReaderAt(StatReaderAt) (SizeReaderAt, error)

In this case, SizeReaderAt would just be interface { ReaderAt; Size() int64 }

This would allow an os.File to be used fairly conveniently as an os.SizeReaderAt without adding any methods to existing types.

Implement Stat method on {bytes,strings}.Reader

bytes.Reader and strings.Reader can be given a stat method which returns a mostly-stubbed os.FileInfo. This would make those types more file-like, but increase the complexity of the interfaces that would come out of this (io.StatReaderAt). A func NewSizeStat(size int64) os.FileInfo or func NewStat(struct { Size int64; ... }) os.FileInfo could be added somewhere to make stubbed Stat methods trivial to write.

This would also make it easier for strings.Reader and other types to be used in implementing http.File and similar.

Multiple incompatible interfaces for the same method name

Any code accepting a "Size" method should check for both Size() int64 and Size() (int64, error) implementations; io could contain a type Sizer interface { Size() (int64, error) } and provide a utility function to aid compatibility:

// Size has the following behaviors:
// - Size(interface { Size() (sz int64, err error) }) -> (sz, err)
// - Size(interface { Size() (sz int64) }) -> (sz, nil)
// - Size(interface { Len() (sz int) }) -> (int64(sz), nil)
func Size(interface{}) (int64, error)

Alternatively, or in addition, a NewSizer(interface{}) io.Sizer could be provided. vet could warn if inputs to these functions do not implement any of the accepted interfaces.

Make a new method, with a new name

ByteLen() (int64, error) for example. This would allow bytes.Reader, strings.Reader, and os.File to all gain a unified method with the expected semantics, without breaking compatibility. This would be the third method for bytes.Reader and strings.Reader, however.

Give os.File conventional Stat method, but also Err() error

Make os.File stateful, similar to bufio.Scanner, such that the Err method acts as a surrogate for a missing error return value in Size.

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 7, 2016
@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 18, 2016
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.9Early May 3, 2017
@bradfitz bradfitz added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jun 14, 2017
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017
@nightlyone
Copy link
Contributor Author

@extemporalgenome I completely missed your excellent analysis here. Thanks for it!

I would like to go with your first suggestion: func NewSizeReaderAt(StatReaderAt) (io.SizeReaderAt, error) and would put that small helper into package os, if needed.

But I believe it might also be an excellent example for the io.SizeReaderAt and doesn't need to become API at all.

Anyway: Adding io.SiteReaderAt shouldn't be blocked by this.

@rsc / @bradfitz Would you like me to send a CL re-adding the interface? Would you accept another CL adding a functionality similiar to NewSizeReaderAt in io as an example for using an os.File to create a io.SizeReaderAt?

@dsnet
Copy link
Member

dsnet commented Sep 14, 2017

Of the approaches listed above, I gravitate most towards just adding a new method. Even then, I don't get warm feelings about expanding the API.

The other approaches either lose type safety or couple os-related functionality into io.

@rsc
Copy link
Contributor

rsc commented Sep 14, 2017

Still don't see a compelling argument for this. This interface can be defined outside the standard library equally well as inside. I'd prefer to just close this, at least until we're rethinking io more generally.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants