Skip to content

New msgpack api (2.11 and 2.10) #3605

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

Merged
merged 1 commit into from
Aug 8, 2023
Merged

New msgpack api (2.11 and 2.10) #3605

merged 1 commit into from
Aug 8, 2023

Conversation

andreyaksenov
Copy link
Contributor

@andreyaksenov andreyaksenov commented Aug 2, 2023

Documented the following API members:

Note that updated docs reference testable code snippets for making sure the code shown in docs is correct.

@andreyaksenov andreyaksenov linked an issue Aug 2, 2023 that may be closed by this pull request
@andreyaksenov andreyaksenov force-pushed the 2.11-msgpack-api branch 12 times, most recently from 3194898 to 5ee66d1 Compare August 4, 2023 14:57
@andreyaksenov andreyaksenov force-pushed the 2.11-msgpack-api branch 3 times, most recently from a16772e to 313e50d Compare August 7, 2023 08:49
@andreyaksenov andreyaksenov changed the title New msgpack api New msgpack api (2.11 and 2.10) Aug 7, 2023
@@ -241,7 +236,7 @@ Below is a list of all ``msgpack`` functions and members.

.. function:: decode_array_header(byte-array, size)

Call the mp_decode_array function in the `MsgPuck <http://rtsisyk.github.io/msgpuck/>`_ library
Call the `MsgPuck <https://rtsisyk.github.io/msgpuck/>`_'s ``mp_decode_array`` function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth mentioning here that this function expects the data to be a MsgPack array. If it isn't, an exception is raised. Same comment for decode_map_header.

@@ -528,7 +538,7 @@ userdata/cdata, you can use this code:
.. NOTE::

To achieve the same effect for only one call to ``msgpack.encode()``
(i.e. without changing the configuration permanently), you can use
(that is without changing the configuration permanently), you can use
``msgpack.encode({1, x, y, 2}, {encode_invalid_numbers = true})``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't true:

tarantool> msgpack.encode({1, 2}, {encode_invalid_numbers = true})
---
- error: 'msgpack.encode: argument 2 must be of type ''struct ibuf'''
...

It's possible to do this though:

msgpack.new({encode_invalid_numbers = true}).encode({1, 2})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Also found out the the example above with httpc.curl doesn't work:

tarantool> msgpack.encode(httpc.curl)
---
- error: unsupported Lua type 'userdata'
...

tarantool> msgpack.encode(httpc.curl, {encode_use_tostring=true})
---
- error: 'msgpack.encode: argument 2 must be of type ''struct ibuf'''
...

Changed to msgpack.cfg{encode_use_tostring = true}.

* if a MsgPack object is an associative array, ``key`` is the string value that specifies the element key.

:return: an element of the MsgPack array.
If the specified key is missing in the array, `get` returns ``nil``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth mentioning that this function raises an error in case the value stored in the object isn't a MsgPack array or map.

local first = mp_map_iterator:decode() -- returns 'foo'
local second = mp_map_iterator:decode() -- returns 'a'
mp_map_iterator:skip() -- returns none, skips 'bar'
local fourth = mp_map_iterator:decode() -- returns ['b1', 'b2', 'b3']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, map elements will not necessarily be returned in this order. A Lua map is unordered so the order in which it's encoded in MsgPack depends on Lua internals. It may be better to rework the example to put decoded elements back into a map/set.

.. code-block:: lua
.. literalinclude:: /code_snippets/test/msgpack/msgpack_object_test.lua
:language: lua
:lines: 1,29-32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you forgot line 5: line 31 refers to mp_from_string defined at line 5.

It's difficult to review these examples. IMO better inline them in the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed. We decided to use testable examples to make sure we don't miss any breaking changes that might break the code snippets used in docs. Looks like more 'atomic' examples that show only the minimal API usage would be more convenient for review - msgpack_object_test.lua actually mixes examples that can be split up.

local size = mp_array_iterator:decode_array_header() -- returns 3
local first = mp_array_iterator:decode() -- returns 10
mp_array_iterator:skip() -- returns none, skips 20
local mp_array_new = mp_array_iterator:take()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it called mp_array_new? It's a MsgPack object that contains 30, i.e. it doesn't have anything to with MsgPack arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mixed up with take_array, thanks for pointing out

@andreyaksenov andreyaksenov force-pushed the 2.11-msgpack-api branch 5 times, most recently from fb14e6f to 0519abf Compare August 7, 2023 13:02
@andreyaksenov andreyaksenov force-pushed the 2.11-msgpack-api branch 3 times, most recently from 7918deb to 4f48bae Compare August 7, 2023 14:21
@andreyaksenov andreyaksenov force-pushed the 2.11-msgpack-api branch 2 times, most recently from 3f1c765 to 755768e Compare August 8, 2023 07:32
@andreyaksenov andreyaksenov requested a review from xuniq August 8, 2023 07:57

* - :ref:`msgpack.decode_map_header(byte-array, size) <msgpack-decode_map_header>`
- Skip a map header in a raw MsgPack string
- Call the `MsgPuck <https://rtsisyk.github.io/msgpuck/>`_'s ``mp_decode_map`` function and return the map size and a pointer to the first map component.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Call the `MsgPuck <https://rtsisyk.github.io/msgpuck/>`_'s ``mp_decode_map`` function and return the map size and a pointer to the first map component.
- Call the `MsgPuck <https://rtsisyk.github.io/msgpuck/>`_'s ``mp_decode_map`` function and return the map size and a pointer to the first map component

@@ -301,8 +305,8 @@ The MsgPack output structure can be specified with the ``__serialize`` parameter

* 'seq', 'sequence', 'array' - table encoded as an array
* 'map', 'mappping' - table encoded as a map
* function - the meta-method called to unpack serializable representation
of table, cdata or userdata objects
* function - the meta-method called to unpack the serializable representation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* function - the meta-method called to unpack the serializable representation
* function -- the meta-method called to unpack the serializable representation

Same for lines 306-307

excessively sparse arrays will be handled as maps.

**msgpack.cfg() example 1:**

If ``msgpack.cfg.encode_invalid_numbers = true`` (the default),
then NaN and Inf are legal values. If that is not desirable, then
ensure that ``msgpack.encode()`` will not accept them, by saying
ensure that ``msgpack.encode()`` do not accept them, by saying
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ensure that ``msgpack.encode()`` do not accept them, by saying
ensure that ``msgpack.encode()`` does not accept them, by saying


The index key used to get the array element might be one of the following:

* if a MsgPack object is an array, ``key`` is the integer value (start with 1) that specifies the element index.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* if a MsgPack object is an array, ``key`` is the integer value (start with 1) that specifies the element index.
* if a MsgPack object is an array, the ``key`` is an integer value (starting with 1) that specifies the element index.


:param number/string key: the index key used to get the array element, which might be one of the following:

* if a MsgPack object is an array, ``key`` is the integer value (start with 1) that specifies the element index.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* if a MsgPack object is an array, ``key`` is the integer value (start with 1) that specifies the element index.
* if a MsgPack object is an array, the ``key`` is an integer value (starting with 1) that specifies the element index.

@andreyaksenov andreyaksenov merged commit d21a9a7 into latest Aug 8, 2023
@andreyaksenov andreyaksenov deleted the 2.11-msgpack-api branch August 8, 2023 11:43
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.

Document msgpack.object:index Document msgpack object iterator take_array method Document msgpack.cfg.encode_error_as_ext
4 participants