bpo-29741: make some BytesIO methods accept integer types by orenmn · Pull Request #560 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits9 Checks0 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
according to http://bugs.python.org/issue29741:
- change some functions in Modules/_io/stringio.c and in Modules/_io/bytesio.c to use _PyIO_ConvertSsize_t (which supports integer types)
- change _io_BytesIO_truncate_impl so that it would accept integer types
- add tests to test_memoryio
- change stuff in Lib/_pyio.py to behave like the C implementaiton
(I ran the test module again, and on my Windows 10, the same tests failed with
and without my patches. However, on my Ubuntu 16.04 VM, none of the tests
failed.)
https://bugs.python.org/issue29741
with regard to continuous-integration/appveyor/pr build fail - I see in the logs that test_site failed, but on my Windows 10, test_site passed.
(6 tests did fail on my Windows 10, but they failed both on cpython with and without my patches.)
Does the AppVeyor build fail prevent core devs from reviewing this patch?
And if it does, what can I do about it?
merged changes from PR #606 and PR #650.
that leaves this PR with changes only in Lib/_pyio.py and Lib/test/test_memoryio.py.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like with the native module changes the rest of the _pyio.py changes may be unnecessary. The enhanced tests are definitely valuable, but it would be nice to see if they pass without adding the extra error messages to _pyio.
| try: |
|---|
| size = size.__index__() |
| except AttributeError as err: |
| raise TypeError("an integer is required") from err |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how size gets used after this, there shouldn't be any need to reassign it by calling __index__. Similarly for the other changes.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I remove the reassignment, I get TypeError: '<' not supported between instances of 'int' and 'IntLike' somewhere later, when the size var is compared to an int.
Did I misunderstand your comment?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steve?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - GitHub messages go to my work email, which means I don't see them unless I'm sneakily doing Python stuff on work time. Conversation on the bug is better.
There shouldn't be any need for the from err - chaining will happen automatically (that syntax is there for explicit non-trivial chaining). It's probably also a good idea to separate the attribute access from the invocation to ensure we wrap up the right exception:
try: size_index = size.index except AttributeError: raise TypeError(f"{size!r} is not int-like enough") else: size = size_index() # now AttributeErrors from inside index will be raised cleanly
I am really busy nowadays (this is my 2nd term), and so I won't be able to work on CPython until August.. if anyone wishes to take it from where I left, please do :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some relatively minor style fixes, but we need to get these right within the stdlib.
| try: |
|---|
| size = size.__index__() |
| except AttributeError as err: |
| raise TypeError("an integer is required") from err |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - GitHub messages go to my work email, which means I don't see them unless I'm sneakily doing Python stuff on work time. Conversation on the bug is better.
There shouldn't be any need for the from err - chaining will happen automatically (that syntax is there for explicit non-trivial chaining). It's probably also a good idea to separate the attribute access from the invocation to ensure we wrap up the right exception:
try: size_index = size.index except AttributeError: raise TypeError(f"{size!r} is not int-like enough") else: size = size_index() # now AttributeErrors from inside index will be raised cleanly
| @@ -11,6 +11,13 @@ |
|---|
| import pickle |
| import sys |
| class IntLike(): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No parens after class names
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad.
BTW, do you think this should be added to PEP8, or is it obvious?
(i grepped and found only 7 places in the codebase doing this.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I found more than that, though they're all in tests. Maybe it's just my personal preference - don't worry about it if you don't want to
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
I didn't expect the Spanish Inquisition!
| try: |
|---|
| size_index = size.__index__ |
| except AttributeError: |
| raise TypeError(f"{size!r} is not an integer") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hope you are OK with this phrasing.. I chose it because even though 'int-like'
is intuitive, it seems that 'integer' is much more common in the docs and in
error messages (including in _pyio.py).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm happy with this now. Just needs the news.d item - click "Details" next to the check below and it'll give you the instructions to add this.
Thanks! I removed my name from the news item, since it doesn't have to go there, and once the build completes I'll merge.
I added your name because most of the code in the patch was written by you.
thanks for the review :)
Heh, maybe, but there's a lot of code in Python that doesn't have my name on it :) That's what happens when you get merge permissions - you stop getting explicit credit for every little change.
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request
jaraco pushed a commit that referenced this pull request
