[tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed May 30 23:49:59 MSK 2018
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 = <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 <v.shpilevoy at tarantool.org>
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 = <string>, "\
- "handle = <string>})");
+ return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
+ "tag_handle = <string>})");
+ }
+ 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}
More information about the Tarantool-patches
mailing list