-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-90533: Implement BytesIO.peek() #30808
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
Open
marcelm
wants to merge
35
commits into
python:main
Choose a base branch
from
marcelm:fix-issue-46375
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+180
−6
Open
Changes from 33 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
b833b83
Implement BytesIO.peek()
marcelm 50a2cfb
📜🤖 Added by blurb_it.
blurb-it[bot] eaa7672
Document BytesIO.peek()
marcelm 00457ae
Implement with the help of read_bytes()
marcelm 882579d
Add to What’s New
marcelm c1eed72
versionadded: 3.12 -> 3.13
marcelm afc200c
Remove unused variable
marcelm 79ab9a4
Test tell() after peek()
marcelm b493914
Update docs, factor out peek_bytes, semantics
marcelm 2a1c85c
Update Misc/NEWS.d/next/Library/2022-04-10-20-10-59.bpo-46375.8j1ogZ.rst
marcelm d398717
Update Modules/_io/bytesio.c
marcelm 26d1e81
Update Modules/_io/bytesio.c
marcelm 9a19ff9
Use SemBr
marcelm 9300ade
Update Doc/whatsnew/3.13.rst
marcelm d214089
Apply suggestions from code review
marcelm d6691b8
Use a context manager around memio in test_peek
marcelm 3e51adb
Add more tests for tell() after peek()
marcelm 3661b65
Document why size < 0 can happen
marcelm cd40d77
Update Modules/_io/bytesio.c
marcelm 04372bd
Do not update pos if peek_bytes failed
marcelm 6b9ae8c
Size can be negative after truncate or seek
marcelm f7406f6
Test with size<0 and size>len(buf)
marcelm d9528e2
Test peek() after write()
marcelm bc8134b
Document BufferedReader.peek and BytesIO.peek similarly
marcelm b6ffca8
Comment
marcelm 5fe5645
Make it more explicit that size is ignored
marcelm 4126a64
Return an empty bytes object for size=0
marcelm 1ea40c2
Simplify
marcelm 77e04d6
Test peek(3) and peek(5)
marcelm 4d2f2dd
Run clinic.py
marcelm c16bebf
Apply suggestions from code review
marcelm 08bd7da
Do not return an empty bytes object for size=0
marcelm 6174fca
Decorate BytesIO.peek with `@critical_section`
marcelm b8b8cf4
Test peek returns EOF after seeking to EOF
marcelm 7ac914e
Free-threading test for peek
marcelm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 1 addition & 0 deletions
1
Misc/NEWS.d/next/Library/2022-04-10-20-10-59.bpo-46375.8j1ogZ.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add :meth:`io.BytesIO.peek`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not default to
size=0likeBufferedReader?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through the comments. In general anything returning a
bytesrather than abytes-likememoryview is going to require allocating and copying potentially many bytes... If the copy is really concerning I'd lean returning a memoryview rather than abyteswhich is a mandatory copy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memoryviewcame up in this comment and the three following it: #30808 (comment)I don’t know what the conclusion is given your comment. Should a memoryview be returned instead? Most important to me is compatibility with what
BufferedReader.peek()returns.I am not too concerned about the extra memory for a
bytesobject as long as the default issize=1.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to not using memoryview / definitely a good rationale in that thread.
On 3.14.0 BufferedReader (intentionally limiting buffer size) gives:
Which doesn't match the behavior here or defaults. That in particular concerns me because it seems like people will build and test I/O stack pieces (ex. file parsing) expecting the
.peekbehavior ofBytesIOthen get something different when they read actual files/data...Looking for more alternatives, the documentation page says (https://docs.python.org/3/library/io.html#io.BufferedReader.peek):
To me that leaves an intentional gap where we can always return less data than the total amount available. Peek gives no guarantees. That makes me wonder: Could we default to 0 and just always slice
[:DEFAULT_BUFFER_SIZE]?That would mean you always get
DEFAULT_BUFFER_SIZEwhich matches defaultBufferedReaderbehavior unless there is less data available. If called in a loop yes that's a DEFAULT_BUFFER_SIZE repeated copy (BufferedIO also does that / thebytesrequires a copy), but it's a lot less than "the whole buffer".