-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
bpo-29741: make some BytesIO methods accept integer types #560
bpo-29741: make some BytesIO methods accept integer types #560
Conversation
@orenmn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @benjaminp, @avassalotti, @vadmium and @rhettinger to be potential reviewers. |
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. |
Does the AppVeyor build fail prevent core devs from reviewing this patch? |
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.
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.
Lib/_pyio.py
Outdated
try: | ||
size = size.__index__() | ||
except AttributeError as err: | ||
raise TypeError("an integer is required") from err |
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.
Given how size
gets used after this, there shouldn't be any need to reassign it by calling __index__
. Similarly for the other changes.
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.
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?
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.
Steve?
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.
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 :) |
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.
Just some relatively minor style fixes, but we need to get these right within the stdlib.
Lib/_pyio.py
Outdated
try: | ||
size = size.__index__() | ||
except AttributeError as err: | ||
raise TypeError("an integer is required") from err |
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.
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
Lib/test/test_memoryio.py
Outdated
@@ -11,6 +11,13 @@ | |||
import pickle | |||
import sys | |||
|
|||
class IntLike(): |
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.
No parens after class names
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.
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.)
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.
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
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.
not worried at all, and I would certainly follow your advice.
just wondered whether this should be added to the PEP..
and it seems that the docs think like you - https://docs.python.org/3.7/tutorial/classes.html#class-definition-syntax.
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! |
Nobody expects the Spanish Inquisition! @zooba: please review the changes made to this pull request. |
try: | ||
size_index = size.__index__ | ||
except AttributeError: | ||
raise TypeError(f"{size!r} is not an integer") |
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.
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).
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.
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. |
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. |
…eger types. Patch by Oren Milman. (python#560)
Bumps [sentry-sdk](https://github.com/getsentry/sentry-python) from 1.5.12 to 1.6.0. - [Release notes](https://github.com/getsentry/sentry-python/releases) - [Changelog](https://github.com/getsentry/sentry-python/blob/master/CHANGELOG.md) - [Commits](getsentry/sentry-python@1.5.12...1.6.0) --- updated-dependencies: - dependency-name: sentry-sdk dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ezio Melotti <[email protected]>
according to http://bugs.python.org/issue29741:
(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