[tarantool-patches] Re: [PATCH v2 1/4] app: serializers update now is reflected in Lua

[Alexander Turenko]

LGTM.

Small comments are below.

WBR, Alexander Turenko.

>  src/lua/utils.c                      | 34 +++++++++++++++-------------
>  test/app-tap/json.test.lua           |  3 ++-
>  test/app-tap/lua/serializer_test.lua | 13 +++++++++++
>  test/app-tap/msgpack.test.lua        |  3 ++-

It looks that it worth to add the test to test/app-tap/yaml.test.lua
too.

>  4 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 75efe0ed2..a082a2e5b 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -270,10 +270,8 @@ luaL_serializer_parse_option(struct lua_State *L, int i,
>  			     struct luaL_serializer *cfg)

Now the function pushes a value to a Lua stack, I think it worth to
update the doxygen-style comment.

>  /**
> - * @brief serializer.cfg{} Lua binding for serializers.
> - * serializer.cfg is a table that contains current configuration values from
> - * luaL_serializer structure. serializer.cfg has overriden __call() method
> - * to change configuration keys in internal userdata (like box.cfg{}).
> - * Please note that direct change in serializer.cfg.key will not affect
> - * internal state of userdata.

The note is still actual, I would not remove it.

( Sooner or later we should just do
https://github.com/tarantool/tarantool/issues/2867 )

Side note: I think that it is better to have one source of information
and have only a handle in Lua. Or maybe (if option(s) access cost is
matter) update the Lua table using some kind of triggers: if we'll
change an option from C, then the current implementation will fail to
track it.

Anyway, it is not a part of your issue. Just side thoughts.