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

[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.
> 
> The test is called 'test_depth' and YAML serializer does not check max
> depth. This is why I omitted it for YAML, deliberately. Next commits
> add here more depth-specific tests, and they will fail for YAML.

Okay.

> >>  /**
> >> - * @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.
> 
> The old comment is super overcomplicated and obvious I think. But ok:
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 6789a8477..93e00d81f 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -302,9 +302,15 @@ luaL_serializer_parse_options(struct lua_State *L,
>  }
>  
>  /**
> - * Serializer.cfg.__call implementation. Merge new parameters into
> - * both C structure of the serializer, and into its Lua table
> - * representation: serializer.cfg.
> + * @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. Changes via cfg() are reflected in
> + * both Lua cfg table, and C serializer structure.
> + * @param L lua stack
> + * @return 0
>   */
>  static int
>  luaL_serializer_cfg(struct lua_State *L)

I said about the note that starts with 'Please note', but I'm okay as
with your comment as well as the original one if the note about the
pitfall with serializer.cfg.key will be anywhere around.

Let's change (or keep) it is the way that looks better for you.

> 
> > 
> > ( 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.
> 
> We could copy luaL_serializer to ffi.cdef so as Lua would be able to work
> with that structure directly. That looks like a piece of cake.

Great! Will you file an issue?