Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
hauntsaninja merged 11 commits intopython:mainfromcmaloney:fileio_comments
Mar 7, 2025

Conversation

cmaloney
Copy link
Contributor

@cmaloneycmaloney commentedJan 19, 2025
edited by bedevere-appbot
Loading

Could this get theno 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

@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 commentedJan 19, 2025
edited
Loading

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 usinghelp()). 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 therst 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 usinghelp()). 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 therst docs or not, or if there is something else that was overlooked.

I apologize, I got my information from thedevguide 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

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 addskip 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 theskip news label. However, we need to check that it's indeed the case before deciding so.

Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
@cmaloney
Copy link
ContributorAuthor

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 theio 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 changingread andreadall 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 ingh-129005. I can update the_pyio comments here, but that will make git conflicts that need resolving.

@picnixz
Copy link
Member

The docs forFileIO assume that they should be the same as forRawIOBase since this class inherits fromRawIOBase. So whatever description forreadall() we have, if there is something to be changed forFileIO.readall because it's different fromRawIOBase.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 canskip news this PR but ideally online docs should be synchronized and reflect the behaviour of_io.<class>.<meth>.

cmaloney reacted with thumbs up emoji

@picnixzpicnixz self-requested a reviewMarch 1, 2025 19:15
@cmaloney
Copy link
ContributorAuthor

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 issuegh-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
ContributorAuthor

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

StanFromIreland reacted with thumbs up emoji

@hauntsaninjahauntsaninjaenabled auto-merge (squash)March 6, 2025 23:52
@cmaloney
Copy link
ContributorAuthor

regenerating clinic and pushing shortly

auto-merge was automatically disabledMarch 7, 2025 00:26

Head branch was pushed to by a user without write access

@hauntsaninjahauntsaninja merged commit886a4d7 intopython:mainMar 7, 2025
41 checks passed
@cmaloneycmaloney deleted the fileio_comments branchMarch 7, 2025 01:19
@cmaloney
Copy link
ContributorAuthor

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

ZeroIntensity reacted with rocket emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@StanFromIrelandStanFromIrelandStanFromIreland left review comments

@hauntsaninjahauntsaninjahauntsaninja approved these changes

@ZeroIntensityZeroIntensityZeroIntensity approved these changes

@picnixzpicnixzAwaiting requested review from picnixz

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@cmaloney@StanFromIreland@picnixz@hauntsaninja@ZeroIntensity

[8]ページ先頭

©2009-2025 Movatter.jp