Re: [tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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@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}