Skip to content
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

Added support for bytes in sliced __setitem__ #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hl037
Copy link

@hl037 hl037 commented Jul 7, 2020

  • Added support for bytes in sliced setitem
  • Test change and addition accordingly
  • Modification of misleading exception message

All tests pass

@bialix
Copy link
Member

bialix commented Jul 7, 2020

I see. OK, that's good addition. But can you explain why we need to support only bytes here? What about bytearray, array.array('B', ...) and memoryview?
https://docs.python.org/3/library/stdtypes.html#binary-sequence-types-bytes-bytearray-memoryview

If you dip your toes here can we get full support please?

Also I need a NEWS.rst entry and you can add yourself to AUTHORS.txt.

@hl037
Copy link
Author

hl037 commented Jul 7, 2020

Actually, I think since I rarely bytearays, and even less array.array, I guess, I just didn't though about it.

To be even more general, I propose I replace by this if you think it's acceptable :

if not isinstance(byte, (bytes, bytearray, list, tuple)):
    try:
        byte = bytes(byte)
    except TypeError as exc:
        raise ValueError('Slice operation expects either a bytes, a sequence of byte values, or anything convertible to bytes') from exc
    except ValueError as exc:
        raise ValueError('Only values in range(0,256) are supported') from exc

...I could also try to convert the list to bytes so that it does value checking too, but I don't want to impact the performance of existing code. What do you think about it ?

By the way, If you are interested, I can also implement the Slice setitem for 16bit

@bialix
Copy link
Member

bialix commented Jul 8, 2020

I think you can omit the isinstance check at all. It's not really needed if we want to be as generic as possible. In fact all that code really need is some iterable object that provide integers. So you can simply change check to something like:

if not isinstance(byte[0], int): raise ValueError("bytes sequence expected")

and that will cover much more use cases. I just want you to make a proper test case when regular string is passed (e.g. "foo" not a byte string).

@hl037
Copy link
Author

hl037 commented Sep 9, 2020

Hello, sorry for the long response time, I was quite busy this summer :)

So, I did not removed all the checks, but I did something that should handle all the cases with minimal tradeoff on performances.

In the case of a bytes or byte array, no conversions / checks occurs on the data. On list and tuples, only value check (int + 0<=x<=255), and on other types, I try to convert to bytes that does the necessary check itself while providing a getitem ready container.

I added my name on the authors and also added a news entry as you suggested. I also let myself increase the version number since it seems to be the way it is, let me know if I should change anything else :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants