Skip to content

gh-129011: Update comments in FileIO to match current code #129012

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

Merged
merged 11 commits into from
Mar 7, 2025

Conversation

cmaloney
Copy link
Contributor

@cmaloney cmaloney commented Jan 19, 2025

Could this get the no news tag? Just updates code comments / documentation to match current implementation.

@StanFromIreland
Copy link
Contributor

@picnixz This one has yet to been labeled, it must have slipped by.

@picnixz
Copy link
Member

picnixz commented Jan 19, 2025

@StanFromIreland Please, let the core devs or the triagers label the PRs when they can. For instance, if the PR has been opened for quite a long time and no one has seen it, then it would be fine to ping someone however it has only been 8h that it has been opened.

@picnixz
Copy link
Member

picnixz commented Jan 19, 2025

In addition, I do think that this one may require a small NEWS entry. The reason is that we're changing something at the C level, namely something that a user will not be able to easily see when inspecting the source code from an IDE (but they can see the new docs using help()). If it were the Python implementation, then a skip news is probably fine but I would like to know if the new docs actually match the rst docs or not, or if there is something else that was overlooked.

@StanFromIreland
Copy link
Contributor

@picnixz

readall()
Read and return all the bytes from the stream until EOF, using multiple calls to the stream if necessary.

read()
Read up to size bytes from the object and return them. As a convenience, if size is unspecified or -1, all bytes until EOF are returned. Otherwise, only one system call is ever made. Fewer than size bytes may be returned if the operating system call returns fewer than size bytes.

If 0 bytes are returned, and size was not 0, this indicates end of file. If the object is in non-blocking mode and no bytes are available, None is returned.

The default implementation defers to [readall()](https://docs.python.org/3/library/io.html#io.RawIOBase.readall) and [readinto()](https://docs.python.org/3/library/io.html#io.RawIOBase.readinto).

docs.python.org/3/library/io.html#io.RawIOBase.read

@StanFromIreland
Copy link
Contributor

In addition, I do think that this one may require a small NEWS entry. The reason is that we're changing something at the C level, namely something that a user will not be able to easily see when inspecting the source code from an IDE (but they can see the new docs using help()). If it were the Python implementation, then a skip news is probably fine but I would like to know if the new docs actually match the rst docs or not, or if there is something else that was overlooked.

I apologize, I got my information from the devguide where it is written that "documentation changes" and "strictly internal changes with no user-visible effects" do not require a news entry and I thought therefore that since this PR is both of those reasons that the "Skip NEWS" is the label for this PR. Maybe a clarification should be added to the devguide about clinic inputs.

@picnixz
Copy link
Member

picnixz commented Jan 19, 2025

Maybe a clarification should be added to the devguide about clinic inputs.

I think the devguide should rather be taken as the "base rules" but then rules can be broken (for a better outcome). Sometimes, we also add skip news to PRs that are internals and not user-facing. However, before adding any label, my main question is more: is the documentation of FileIO now synchronized across C implementation, Python implementation and online docs? Usually, the online docs are the reference because that's what users see in the first place. So if the latter do not need to be modified, then we can add the skip news label. However, we need to check that it's indeed the case before deciding so.

Co-authored-by: Stan Ulbrych <[email protected]>
@cmaloney
Copy link
Contributor Author

Trying to learn around skip news myself. I don't think the _io.FileIO.<function> docs show up in the docs.

Neither _pyio or _io show up in the io library doc though, rather that's hand-written separately: https://github.com/python/cpython/blob/main/Doc/library/io.rst?plain=1#L473-L502. In some ways make me wonder "could this go from three copies of the doc to 1?" But I'd like to keep that out of scope for this change. This came about because I was changing read and readall and found the comment and implementation had diverged a bit.

re: _pyio to _io sync, it looks like in this case they were aligned (https://github.com/python/cpython/blob/main/Lib/_pyio.py#L1636-L1657). The actual behavior differs some and I'm working on that in gh-129005. I can update the _pyio comments here, but that will make git conflicts that need resolving.

@picnixz
Copy link
Member

picnixz commented Jan 19, 2025

The docs for FileIO assume that they should be the same as for RawIOBase since this class inherits from RawIOBase. So whatever description for readall() we have, if there is something to be changed for FileIO.readall because it's different from RawIOBase.readall, then it should likely be documented as well, but we can do that in another PR.

Now, _pyio is mostly used for testing and not for production. Now, I agree that we can skip news this PR but ideally online docs should be synchronized and reflect the behaviour of _io.<class>.<meth>.

@picnixz picnixz self-requested a review March 1, 2025 19:15
@cmaloney
Copy link
Contributor Author

cmaloney commented Mar 1, 2025

I synced up docs of _pyio.FileIO.read and _pyio.FileIO.readall with _io.FileIO equivalents.

Separately I started working on similar BufferedIO cases which has an older issue gh-80050 with #130653

(note for these, not sure what/when/how I should push for merge when they've sat a while)

@StanFromIreland
Copy link
Contributor

You don't have to merge, it will be done by a core-dev if they need to before merging.

@cmaloney
Copy link
Contributor Author

cmaloney commented Mar 1, 2025

Did update branch to get all checks passing / one of the builders had failed because of HTTP 504 Gateway errors. Will try to avoid generally

@hauntsaninja hauntsaninja enabled auto-merge (squash) March 6, 2025 23:52
@cmaloney
Copy link
Contributor Author

cmaloney commented Mar 7, 2025

regenerating clinic and pushing shortly

auto-merge was automatically disabled March 7, 2025 00:26

Head branch was pushed to by a user without write access

@hauntsaninja hauntsaninja merged commit 886a4d7 into python:main Mar 7, 2025
41 checks passed
@cmaloney cmaloney deleted the fileio_comments branch March 7, 2025 01:19
@cmaloney
Copy link
Contributor Author

cmaloney commented Mar 7, 2025

@StanFromIreland , @picnixz , @ZeroIntensity , @hauntsaninja thanks for the reviews and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants