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

Type annotations in msgpack are incorrect #6050

Closed
rimwolf-redux opened this issue Feb 17, 2022 · 8 comments
Closed

Type annotations in msgpack are incorrect #6050

rimwolf-redux opened this issue Feb 17, 2022 · 8 comments

Comments

@rimwolf-redux
Copy link

In circuitpython.readthedocs.io, the API documentation for msgpack.pack gives the buffer parameter the type annotation circuitpython_typing.WriteableBuffer. bytearray is listed as one of the types that implements that protocol. However, this code fails:

>>> import msgpack
>>> buf = bytearray(20)
>>> msgpack.pack([2,3], buf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: stream operation not supported

msgpack.unpack has a similar issue with buffer: ReadableBuffer.

The types that are evidently acceptable here are io.BytesIO and FileIO. The implementation (https://github.com/adafruit/circuitpython/blob/main/shared-module/msgpack/__init__.c) makes it clear that the buffer object must have read and write methods, so I suppose the argument is a "file-like object". It's unclear to me what the appropriate type annotation should be.
(The annotations are provided in the corresponding shared-bindings C file.)

I tried this in 7.1.0 on Clue, Feather M4 Express, and Feather S2 TFT, and 7.2.0-alpha2 on M4 Express.

@dhalbert dhalbert added this to the 7.x.x milestone Feb 18, 2022
@tannewt tannewt added the bug label Feb 18, 2022
@tannewt
Copy link
Member

tannewt commented Feb 18, 2022

The simplest type annotation is probably a Union of BytesIO and FileIO. I'm not too aware of the type options though.

@rimwolf-redux
Copy link
Author

Other objects which work for buffer are of type busio.UART and usb_cdc.Serial. (So the msgpack format would a good basis for a binary serial protocol.)
I don't know of any type annotation in standard Python or CircuitPython that encompasses all these types. Perhaps the best approach would be to annotate with a pseudo-type like "bytestream" and explain what that means in the class or method documentation (in shared-bindings/msgpack/__init__.c) I'm willing to wordsmith a PR if this seems like a good approach.

@dhalbert
Copy link
Collaborator

We have circuitpython_typing.ReadableBuffer and .WriteableBuffer in circuitpython_typing (https://github.com/adafruit/Adafruit_CircuitPython_Typing), which are inspired by python/typing#593.

@prplz
Copy link

prplz commented Feb 21, 2022

ReadableBuffer and WritableBuffer are objects that allow accessing bytes by index and a length, these types do not work with msgpack. Msgpack requires something more like a WritableStream and ReadableStream. You can search for STATIC const mp_stream_p_t to find the types that implement stream.

@dhalbert
Copy link
Collaborator

Sorry, I did not read carefully enough. Something like https://stackoverflow.com/questions/41748896/the-correct-way-to-annotate-a-file-type-in-python. If you would like to add such a type to circuitpython_typing, that would be fine.

@tannewt
Copy link
Member

tannewt commented Feb 22, 2022

Note that we already have a "stream" protocol internally and it'd be good to have a corresponding Python type for it.

@rimwolf-redux
Copy link
Author

There's a pull request (adafruit/Adafruit_CircuitPython_Typing#2) to add a ByteStream protocol.

@dhalbert
Copy link
Collaborator

dhalbert commented Mar 1, 2022

Fixed by #6085.

@dhalbert dhalbert closed this as completed Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants