Added support for bytes in sliced __setitem__ by hl037 · Pull Request #42 · python-intelhex/intelhex · GitHub
Skip to content

Added support for bytes in sliced __setitem__#42

Open
hl037 wants to merge 2 commits into
python-intelhex:masterfrom
hl037:master
Open

Added support for bytes in sliced __setitem__#42
hl037 wants to merge 2 commits into
python-intelhex:masterfrom
hl037:master

Conversation

@hl037

@hl037 hl037 commented Jul 7, 2020

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

All tests pass

@bialix

bialix commented Jul 7, 2020

Copy link
Copy Markdown
Member

@hl037

hl037 commented Jul 7, 2020

Copy link
Copy Markdown
Author

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

bialix commented Jul 8, 2020

Copy link
Copy Markdown
Member

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

hl037 commented Sep 9, 2020

Copy link
Copy Markdown
Author

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