bpo-29741: make some BytesIO methods accept integer types by orenmn · Pull Request #560 · python/cpython

orenmn · 2017-03-08T12:35:45Z

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

mention-bot · 2017-03-08T12:35:48Z

@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.

orenmn · 2017-03-08T16:32:45Z

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.)

orenmn · 2017-03-10T11:02:07Z

Does the AppVeyor build fail prevent core devs from reviewing this patch?
And if it does, what can I do about it?

orenmn · 2017-03-13T08:26:45Z

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.

zooba

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.

zooba · 2017-05-24T16:36:10Z

+            try:
+                size = size.__index__()
+            except AttributeError as err:
+                raise TypeError("an integer is required") from err


Given how size gets used after this, there shouldn't be any need to reassign it by calling __index__. Similarly for the other changes.

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?

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

orenmn · 2017-05-24T18:46:20Z

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 :)

zooba

Just some relatively minor style fixes, but we need to get these right within the stdlib.

zooba · 2017-08-21T17:53:55Z

+            try:
+                size = size.__index__()
+            except AttributeError as err:
+                raise TypeError("an integer is required") from err


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

zooba · 2017-08-21T17:54:03Z

 import pickle
 import sys

+class IntLike():


No parens after class names

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.)

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

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.

bedevere-bot · 2017-08-21T18:01:23Z

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.

orenmn · 2017-08-22T17:35:54Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-08-22T17:35:56Z

Nobody expects the Spanish Inquisition!

@zooba: please review the changes made to this pull request.

orenmn · 2017-08-22T17:36:08Z

+            try:
+                size_index = size.__index__
+            except AttributeError:
+                raise TypeError(f"{size!r} is not an integer")


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).

zooba

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.

zooba · 2017-08-24T18:11:44Z

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.

orenmn · 2017-08-24T18:17:03Z

I added your name because most of the code in the patch was written by you.
thanks for the review :)

zooba · 2017-08-24T18:20:28Z

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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>

orenmn added 2 commits March 8, 2017 01:53

patch init commit

08e40b2

changed arg name of _io_StringIO_readline_impl

1ab4836

the-knights-who-say-ni added the CLA signed label Mar 8, 2017

brettcannon added the type-feature A feature request or enhancement label Mar 8, 2017

orenmn added 2 commits March 13, 2017 09:59

commit changes from PR python#650 and python#606

4beafeb

merged master

50d521d

zooba reviewed May 24, 2017

View reviewed changes

merged master.

fca4a4c

bedevere-bot added the awaiting core review label Aug 21, 2017

zooba requested changes Aug 21, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Aug 21, 2017

orenmn added 2 commits August 22, 2017 16:24

merge master

f52a7bd

updates according to zooba's review

ccb19ae

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 22, 2017

orenmn commented Aug 22, 2017

View reviewed changes

zooba approved these changes Aug 22, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 22, 2017

orenmn and others added 2 commits August 23, 2017 00:34

add NEWS.d item

7a5366d

Remove name from NEWS item.

3e831f7

zooba merged commit de50360 into python:master Aug 24, 2017

Mariatta removed the awaiting merge label Aug 29, 2017

GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017

bpo-29741: Update some methods in the _pyio module to also accept int…

d8e4533

…eger types. Patch by Oren Milman. (python#560)

Uh oh!

Conversation

orenmn commented Mar 8, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 8, 2017

Uh oh!

orenmn commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orenmn commented Mar 10, 2017

Uh oh!

orenmn commented Mar 13, 2017

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenmn commented May 24, 2017

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 21, 2017

Uh oh!

orenmn commented Aug 22, 2017

Uh oh!

bedevere-bot commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

zooba commented Aug 24, 2017

Uh oh!

orenmn commented Aug 24, 2017

Uh oh!

zooba commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

orenmn commented Mar 8, 2017 •

edited by bedevere-bot

Loading

orenmn commented Mar 8, 2017 •

edited

Loading