From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged References: <7c30b9483a38bd36425d7e0c87b4d15e8893446e.1524228894.git.v.shpilevoy@tarantool.org> <20180510182210.GB30593@atlas> <20180530191520.GH18850@atlas> From: Vladislav Shpilevoy Message-ID: <1efb7fce-8170-c3db-d422-916f64855651@tarantool.org> Date: Wed, 30 May 2018 23:49:59 +0300 MIME-Version: 1.0 In-Reply-To: <20180530191520.GH18850@atlas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: Konstantin Osipov Cc: tarantool-patches@freelists.org, vdavydov.dev@gmail.com List-ID: Hello. Thanks for the review! >>>> +/** >>>> + * Dump a Lua object into YAML string with a global TAG specified. >>> >>> Serialize a Lua object as YAML string, taking into account a >>> global tag, if it's supplied in the arguments. >> >> - * Dump a Lua object into YAML string with a global TAG specified. >> + * Serialize a Lua object as YAML string, taking into account a >> + * global tag. >> >> I did not add "if it's supplied in the arguments", because a tag is >> required. >> >>> >>>> + * @param options First argument on a stack, consists of two >>>> + * options: tag prefix and tag handle. >>>> + * @param object Lua object to dump under the global tag. >>>> + * @retval Lua string with dumped object. >>> >>> Why do you take options first, object second? AFAICS we usually >>> take object first, options second. Let's be consistent? >> >> + * @param object Lua object to dump under the global tag. >> + * @param options Table with two options: tag prefix and tag >> + * handle. >> >>> >>> Which begs the question, why do you need a new function rather >>> than extend an existing one with options? >> >> Original yaml.encode has this signature: encode(...). It takes >> any argument count, and dumps each. So I can not add an option to >> this. Example: >> yaml.encode(object, {prefix = ..., handle = ...}) >> >> What to do, dump both arguments or use the second as options? I think >> that we lost a chance to change encode() signature, it is already public >> and documented, and will always take (...) arguments. >> >> But here it is documented >> https://tarantool.io/en/doc/1.9/reference/reference_lua/yaml.html?highlight=yaml#lua-function.yaml.encode >> that yaml.encode has one argument. Maybe no one noticed that it takes multiple >> values. If you want, I could change its signature from (...) to (value, [{tag = }]). > > Please do. > We can see if anyone complains and revert the change. > > I am pushing the patch. Please make this last remaining change on > a separate branch, maybe independently of the rest of the patch > stack. > Done. See the patch below. It is the first commit on the same branch. ====================================================================== commit d94518c0f084a1e1f35140c8b1dd972037ffddf2 Author: Vladislav Shpilevoy Date: Wed May 30 23:14:44 2018 +0300 lua: merge encode_tagged into encode options Encode_tagged is a workaround to be able to pass options to yaml.encode(). Before the patch yaml.encode() in fact has this signature: yaml.encode(...). So it was impossible to add any options to this function - all of them would be treated as the parameters. But documentation says: https://tarantool.io/en/doc/1.9/reference/reference_lua/yaml.html?highlight=yaml#lua-function.yaml.encode that the function has this signature: yaml.encode(value). I hope if anyone uses yaml.encode(), he does it according to the documentation. And I can add the {tag_prefix, tag_handle} options to yaml.encode() and remove yaml.encode_tagged() workaround. diff --git a/test/app-tap/yaml.test.lua b/test/app-tap/yaml.test.lua index bb75ce4ea..30e94ddd4 100755 --- a/test/app-tap/yaml.test.lua +++ b/test/app-tap/yaml.test.lua @@ -72,20 +72,20 @@ end local function test_tagged(test, s) test:plan(7) local prefix = 'tag:tarantool.io/push,2018' - local ok, err = pcall(s.encode_tagged) - test:isnt(err:find('Usage'), nil, "encode_tagged usage") - ok, err = pcall(s.encode_tagged, 100, {}) - test:isnt(err:find('Usage'), nil, "encode_tagged usage") - ok, err = pcall(s.encode_tagged, 200, {handle = true, prefix = 100}) - test:isnt(err:find('Usage'), nil, "encode_tagged usage") + local ok, err = pcall(s.encode, 200, {tag_handle = true, tag_prefix = 100}) + test:isnt(err:find('Usage'), nil, "encode usage") + ok, err = pcall(s.encode, 100, {tag_handle = 'handle'}) + test:isnt(err:find('Usage'), nil, "encode usage, no prefix") + ok, err = pcall(s.encode, 100, {tag_prefix = 'prefix'}) + test:isnt(err:find('Usage'), nil, "encode usage, no handle") local ret - ret, err = s.encode_tagged(300, {handle = '!push', prefix = prefix}) + ret, err = s.encode(300, {tag_handle = '!push', tag_prefix = prefix}) test:is(ret, nil, 'non-usage and non-oom errors do not raise') - test:is(err, "tag handle must end with '!'", "encode_tagged usage") - ret = s.encode_tagged(300, {handle = '!push!', prefix = prefix}) - test:is(ret, "%TAG !push! "..prefix.."\n--- 300\n...\n", "encode_tagged usage") - ret = s.encode_tagged({a = 100, b = 200}, {handle = '!print!', prefix = prefix}) - test:is(ret, "%TAG !print! tag:tarantool.io/push,2018\n---\na: 100\nb: 200\n...\n", 'encode_tagged usage') + test:is(err, "tag handle must end with '!'", "encode usage") + ret = s.encode(300, {tag_handle = '!push!', tag_prefix = prefix}) + test:is(ret, "%TAG !push! "..prefix.."\n--- 300\n...\n", "encode usage") + ret = s.encode({a = 100, b = 200}, {tag_handle = '!print!', tag_prefix = prefix}) + test:is(ret, "%TAG !print! tag:tarantool.io/push,2018\n---\na: 100\nb: 200\n...\n", 'encode usage') end tap.test("yaml", function(test) diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc index ca7f4d224..a07804a09 100644 --- a/third_party/lua-yaml/lyaml.cc +++ b/third_party/lua-yaml/lyaml.cc @@ -631,8 +631,8 @@ static void find_references(struct lua_yaml_dumper *dumper) { } /** - * Encode an object or objects on Lua stack into YAML stream. - * @param L Lua stack to get arguments and push result. + * Encode an object on Lua stack into YAML stream. + * @param L Lua stack to get an argument and push the result. * @param serializer Lua YAML serializer. * @param tag_handle NULL, or a global tag handle. For global tags * details see the standard: @@ -651,12 +651,11 @@ static void find_references(struct lua_yaml_dumper *dumper) { * object. */ static int -lua_yaml_encode_impl(lua_State *L, struct luaL_serializer *serializer, - const char *tag_handle, const char *tag_prefix) +lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer, + const char *tag_handle, const char *tag_prefix) { struct lua_yaml_dumper dumper; yaml_event_t ev; - int argcount; dumper.L = L; dumper.begin_tag.handle = (yaml_char_t *) tag_handle; @@ -666,15 +665,10 @@ lua_yaml_encode_impl(lua_State *L, struct luaL_serializer *serializer, * If a tag is specified, then tag list is not empty and * consists of a single tag. */ - if (tag_prefix != NULL) { + if (tag_prefix != NULL) dumper.end_tag = &dumper.begin_tag + 1; - /* Only one object can be encoded with a tag. */ - argcount = 1; - } else { + else dumper.end_tag = &dumper.begin_tag; - /* When no tags - encode all the stack. */ - argcount = lua_gettop(L); - } dumper.cfg = serializer; dumper.error = 0; /* create thread to use for YAML buffer */ @@ -694,17 +688,15 @@ lua_yaml_encode_impl(lua_State *L, struct luaL_serializer *serializer, !yaml_emitter_emit(&dumper.emitter, &ev)) goto error; - for (int i = 0; i < argcount; i++) { - lua_newtable(L); - dumper.anchortable_index = lua_gettop(L); - dumper.anchor_number = 0; - lua_pushvalue(L, i + 1); /* push copy of arg we're processing */ - find_references(&dumper); - dump_document(&dumper); - if (dumper.error) - goto error; - lua_pop(L, 2); /* pop copied arg and anchor table */ - } + lua_newtable(L); + dumper.anchortable_index = lua_gettop(L); + dumper.anchor_number = 0; + lua_pushvalue(L, 1); /* push copy of arg we're processing */ + find_references(&dumper); + dump_document(&dumper); + if (dumper.error) + goto error; + lua_pop(L, 2); /* pop copied arg and anchor table */ if (!yaml_stream_end_event_initialize(&ev) || !yaml_emitter_emit(&dumper.emitter, &ev) || @@ -735,41 +727,43 @@ error: } } -/** - * Dump Lua objects into YAML string. It takes any argument count, - * and dumps each in a separate document. - * @retval 1 Pushes one value: string with dumped documents. - */ -static int l_dump(lua_State *L) { - return lua_yaml_encode_impl(L, luaL_checkserializer(L), NULL, NULL); -} - /** * Serialize a Lua object as YAML string, taking into account a - * global tag. + * global tag if specified. * @param object Lua object to dump under the global tag. * @param options Table with two options: tag prefix and tag * handle. * @retval 1 Pushes Lua string with dumped object. * @retval 2 Pushes nil and error message. */ -static int l_dump_tagged(lua_State *L) -{ +static int l_dump(lua_State *L) { struct luaL_serializer *serializer = luaL_checkserializer(L); - if (lua_gettop(L) != 2 || !lua_istable(L, 2)) { + int top = lua_gettop(L); + if (top > 2) { usage_error: - return luaL_error(L, "Usage: encode_tagged(object, {prefix = , "\ - "handle = })"); + return luaL_error(L, "Usage: encode(object, {tag_prefix = , "\ + "tag_handle = })"); + } + const char *prefix = NULL, *handle = NULL; + if (top == 2 && !lua_isnil(L, 2)) { + if (! lua_istable(L, 2)) + goto usage_error; + lua_getfield(L, 2, "tag_prefix"); + if (lua_isstring(L, -1)) + prefix = lua_tostring(L, -1); + else if (! lua_isnil(L, -1)) + goto usage_error; + + lua_getfield(L, 2, "tag_handle"); + if (lua_isstring(L, -1)) + handle = lua_tostring(L, -1); + else if (! lua_isnil(L, -1)) + goto usage_error; + + if ((prefix == NULL) != (handle == NULL)) + goto usage_error; } - lua_getfield(L, 2, "prefix"); - if (! lua_isstring(L, -1)) - goto usage_error; - const char *prefix = lua_tostring(L, -1); - lua_getfield(L, 2, "handle"); - if (! lua_isstring(L, -1)) - goto usage_error; - const char *handle = lua_tostring(L, -1); - return lua_yaml_encode_impl(L, serializer, handle, prefix); + return lua_yaml_encode(L, serializer, handle, prefix); } static int @@ -777,7 +771,6 @@ l_new(lua_State *L); static const luaL_Reg yamllib[] = { { "encode", l_dump }, - { "encode_tagged", l_dump_tagged }, { "decode", l_load }, { "new", l_new }, { NULL, NULL}