gh-90533: Implement BytesIO.peek()#30808
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
marcelm
force-pushed
the
fix-issue-46375
branch
from
3111318 to
675718d
Compare
marcelm
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
marcelm
force-pushed
the
fix-issue-46375
branch
from
675718d to
d4e849f
Compare
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
marcelm
force-pushed
the
fix-issue-46375
branch
from
ef5eb69 to
b82fa85
Compare
marcelm
force-pushed
the
fix-issue-46375
branch
2 times, most recently
from
6998be1 to
be39ff2
Compare
|
@AlexWaygood You’ve been the only human to interact with this PR so far, do you possibly have any advice on how to move this forward? |
|
Hi @marcelm — sorry for the delay in anybody looking at this. I haven't studied your PR in detail (or thought about whether the proposal is a good idea), but it looks well put together at first glance. I'll try to take a look soon. I won't be able to review the C code, but I can comment on whether the proposal seems like a good idea, and I can review the Python implementation and the tests. Note that if this proposal is accepted, it will also need:
You can also add yourself to |
|
Thank you! I pushed a documentation update and will add an entry to the What’s new document in case the PR is reviewed favorably. |
|
I'll defer to @benjaminp's judgement on this one -- I'm really not the right reviewer for this, unfortunately :( Please ping me again if you still haven't had a review in a few weeks. |
|
Thank you! I appreciate that you took the time. |
@AlexWaygood Hi! Been 8 months, no review so far :) How can we take this forward? I'm not the original author of the PR but I also have a use-case for this API and would like to see this change land, happy to help progress this. |
|
Thanks for your interest! I have updated the PR to fix the merge conflict and to reflect that it now needs to target 3.13. |
|
@marcelm That was quick, thank you so much! |
marcelm
force-pushed
the
fix-issue-46375
branch
from
5cb3143 to
79d5032
Compare
|
cc: @erlend-aasland @vstinner @benjaminp for the A |
| .. versionadded:: 3.13 | ||
|
|
||
| Return bytes from the current position onwards but without advancing the | ||
| position. The number of bytes returned may be less or more than requested. |
There was a problem hiding this comment.
Wait. Why can it more than requested?
Apparently size=0 means "read all". Please document it.
There was a problem hiding this comment.
This was just copied from the documentation for BufferedReader.peek(), but I agree that it could be written better. Changed now.
Wait. Why can it more than requested?
BufferedReader.peek() ignores the size argument and just returns whatever it has in its internal buffer.
There was a problem hiding this comment.
I would prefer that BytesIO.peek() documents its behavior, rather than BufferedReader.peek() behavior.
There was a problem hiding this comment.
Absolutely, it’s already changed.
(BTW, is there a policy on who clicks the "Resolve conversation" button?)
| pos = self.tell() | ||
| if size == 0: | ||
| size = -1 | ||
| b = self.read(size) |
There was a problem hiding this comment.
Would it be possible to implement it without touching the position? This code is not thread safe. I don't know if it's supposed to be thread-safe. Maybe add a private read method which has an argument to decide to move the position or not.
Same remark for C code.
There was a problem hiding this comment.
Good point; I implemented this now so that the position is not changed. I factored out a peek_bytes function in the C version that does not advance the position.
It seems though that neither the C nor the Python version of BytesIO are supposed to be thread safe. (They don’t use locks as e.g. BufferedReader does.) So I would suggest making them so would be a task for a different PR.
marcelm
requested review from
a team,
1st1,
DinoV,
Fidget-Spinner,
StanFromIreland,
Yhg1s,
ZeroIntensity,
asvetlov,
brandtbucher,
brettcannon,
corona10,
diegorusso,
ericsnowcurrently,
ericvsmith,
hugovk,
markshannon,
pfmoore,
pganssle,
picnixz,
pradyunsg,
savannahostrowski and
willingc
as code owners
|
All commit authors signed the Contributor License Agreement. |
Oh oh. Something went wrong with this PR :-( I suggest to abandon this PR and create a new one. |
|
Sorry, I merged main manually; it seems this triggered a lot of stuff I did not intend to. |
|
I will open a new PR. |
|
The new PR: #150917. |
|
@marcelm, probably you forgot to pull changes from upstream to your main branch or to your feature branch. |
14 participants
Closes gh-90533