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
BytesIO methods don't accept integer types, while StringIO counterparts do #73927
Comments
------------ current state ------------ import io
class IntLike():
def __init__(self, num):
self._num = num
def __index__(self):
return self._num
__int__ = __index__ io.StringIO('blah blah').read(IntLike(2)) io.BytesIO(b'blah blah').read(IntLike(2)) The three StringIO methods are called without any error, but each of the three This is because the functions which implement the StringIO methods (in However, the functions which implement the BytesIO methods (in ------------ proposed changes ------------
|
What is the use case? Unless changing the behaviour would be useful, I think the simplest solution would be to document that the methods should only be given instances of “int”, so that it is clear that other kinds of numbers are unsupported. |
I don't have a use case for that. (I noticed this behavior by However, IIUC, commit 4fa88fa, And now that you mention the docs, according to them, both StringIO |
Other objects in the io module use special-purposed converter _PyIO_ConvertSsize_t() which checks PyNumber_Check() and calls PyNumber_AsSsize_t(). I think StringIO implementation can be changed to reuse _PyIO_ConvertSsize_t() for simplicity. After that BytesIO implementation can be changed to use the same converter just for uniformity. |
Are there tests for accepting non-integers in other classes? If no then don't bother about tests. I suppose all this came from the implementation of the 'n' format unit in PyArg_Parse* functions. |
using _PyIO_ConvertSsize_t sounds great. with regard to tests for accepting integer types in other classes, |
also with regard to adding tests, consider the following scenario: lastly, ISTM adding these tests would be quite simple anyway. |
I wrote a patch, but I attach it here and not in a PR, as you haven't approved also, while running test_memoryio with my added tests, i noticed some places in |
LGTM. |
should I open a PR (even though we didn't give Florent enough time to |
Open a PR. It will be not hard to make small changes after opening it. |
sure. In general, should drafts (like this one) be uploaded here? |
This for your preference. |
I would accept changes to Modules/_io/ because I consider them as code cleanup (with little side effect). But I'm not sure about changes to Python implementation and tests. Could you create more narrow PR for Modules/_io/ changes and left other changes for the consideration of the io module maintainers? |
done |
thanks :) |
or maybe git is smart enough to realize what happened, and I don't have |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: