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

os.file.Read() documentation should reflect io.Reader documentation #49470

Closed
ghost opened this issue Nov 9, 2021 · 4 comments
Closed

os.file.Read() documentation should reflect io.Reader documentation #49470

ghost opened this issue Nov 9, 2021 · 4 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ghost
Copy link

ghost commented Nov 9, 2021

Currently the documentation for os.file.Read() consists of the following which can be found in source:
image

This is of course mirrored by the pkg.go.dev documentation site:
image

os.file.Read() makes a call to file.read() and in turn file.pfd.Read(), which within source implements io.Reader:
image

The documentation for io.Reader is much more verbose than for os.file.Read():
image

Making it so that os.file.Read() has documentation that is actually not 100% accurate. os.file.Read() states that it reads up to b []bytes and returns n number of bytes read, however because it implements io.Reader, what it actually does is reads up to AND stores in b []bytes what is read, and then returns n number of bytes read.

This ambiguity had me go down a bit of a rabbit hole as you can tell but I'm glad I have the answer as to how a simple expression such as string(b[:n]) actually contains information.

@ianlancetaylor
Copy link
Contributor

I don't think we should mirror the io.Reader documentation for os.File.Read. The documentation for os.File.Read should describe what that method does. The io.Reader documentation should describe the API permitted for any arbitrary Read method. os.File.Read should implement that API, but it doesn't need to copy the full API description.

@gopherbot
Copy link

Change https://golang.org/cl/362335 mentions this issue: os: clarify that File.{Read,Write} use the buffer

@ghost
Copy link
Author

ghost commented Nov 9, 2021

Reflect may have been to strong of a word for the fix needed here. I don't think that os.File.Read needs to have a mirror of the io.Reader documentation, but I do think it should state that the input []bytes are used to store what is read from the file, rather than the current verbiage which is similar to stating 'Read this document, now tell me how many bytes you read', which sounds like an entirely different method ie BytesRead.

@ghost
Copy link
Author

ghost commented Nov 9, 2021

I see the commit now with a fix, thank you.

@cagedmantis cagedmantis added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 9, 2021
@cagedmantis cagedmantis added this to the Backlog milestone Nov 9, 2021
@golang golang locked and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants