| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@picnixz This one has yet to been labeled, it must have slipped by. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
docs.python.org/3/library/io.html#io.RawIOBase.read |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
|
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. |
Sorry, something went wrong.
|
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>. |
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
|
You don't have to merge, it will be done by a core-dev if they need to before merging. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
regenerating clinic and pushing shortly |
Sorry, something went wrong.
Head branch was pushed to by a user without write access
|
@StanFromIreland , @picnixz , @ZeroIntensity , @hauntsaninja thanks for the reviews and merge! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Could this get the no news tag? Just updates code comments / documentation to match current implementation.