GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Added support for bytes in sliced __setitem__ by hl037 · Pull Request #42 · python-intelhex/intelhex

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bialix

bialix commented Jul 7, 2020

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bialix

bialix commented Jul 8, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hl037

hl037 commented Sep 9, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Léo Flaventin Hauchecorne
Added support for bytes in sliced __setitem__
81d5a0d
@hl037
hl037 force-pushed the master branch from c79f949 to 81d5a0d Compare September 9, 2020 02:54
@hl037
Fixed array conversion
94068de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@hl037 @bialix
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.