[Tarantool-patches] [PATCH] box/console: Handle multireturn in lua mode

[Tarantool-patches] [PATCH] box/console: Handle multireturn in lua mode

[Cyrill Gorcunov]

Currently we handle only first member of
multireturn statement. Fix it processing
each element separately.

n.b.: While at this file add vim settings.

 | tarantool> \set output lua
 | true;
 | tarantool> 1,2,3,4
 | 1, 2, 3, 4;

Fixes #4604

Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue: https://github.com/tarantool/tarantool/issues/4604
branch: gorcunov/gh-4604-multireturn

 src/box/lua/console.lua | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index d4d8ec984..e9a9e8261 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -1,4 +1,6 @@
 -- console.lua -- internal file
+--
+-- vim: ts=4 sw=4 et
 
 local serpent = require('serpent')
 local internal = require('console')
@@ -75,28 +77,38 @@ local serpent_map_symbols = function(tag, head, body, tail, level)
     return tag..head..body..tail
 end
 
-output_handlers["lua"] = function(status, opts, ...)
-    --
-    -- If no data present at least EOS should be put,
-    -- otherwise wire readers won't be able to find
-    -- where the end of string is.
-    if not ... then
-        return output_eos["lua"]
-    end
+--
+-- Format element into lua form
+local function format_lua(opts, elem)
     for k,v in pairs(lua_map_direct_symbols) do
-        if k == ... then
-            return v .. output_eos["lua"]
+        if k == elem then
+            return v
         end
     end
     local serpent_opts = {
         custom  = serpent_map_symbols,
         comment = false,
-        nocode = true,
+        nocode  = true,
     }
     if opts == "block" then
-        return serpent.block(..., serpent_opts) .. output_eos["lua"]
+        return serpent.block(elem, serpent_opts)
+    end
+    return serpent.line(elem, serpent_opts)
+end
+
+output_handlers["lua"] = function(status, opts, ...)
+    local collect = { }
+    --
+    -- If no data present at least EOS should be put,
+    -- otherwise wire readers won't be able to find
+    -- where the end of string is.
+    if not ... then
+        return output_eos["lua"]
+    end
+    for i = 1,select('#', ...) do
+        collect[i] = format_lua(opts, select(i, ...))
     end
-    return serpent.line(..., serpent_opts) .. output_eos["lua"]
+    return table.concat(collect, ', ') .. output_eos["lua"]
 end
 
 local function output_verify_opts(fmt, opts)
-- 
2.20.1

Re: [Tarantool-patches] [PATCH] box/console: Handle multireturn in lua mode

[Alexander Turenko]

Looks good in general and works correct even with nils at the end.

We however should add a test case for this, otherwise sooner or later
we'll unable to make changes w/o fear to broke something. I propose to
add a case with nils at the end as well as simple 1, 2, 3 multireturn.

I put several nits below. Some of them are pointed by our [Lua Style
Guide][1], another ones are what I see across other parts of our
codebase. My intention here is to keep the style more or less consistent
and so make it more comfortable to work with the code (hope not only for
me, otherwise it is not productive).

[1]: https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/

WBR, Alexander Turenko.

> box/console: Handle multireturn in lua mode

Nit: we usually start a commit message header from a small letter after
a prefix.

> Currently we handle only first member of
> multireturn statement. Fix it processing
> each element separately.
> 
> n.b.: While at this file add vim settings.
> 
>  | tarantool> \set output lua
>  | true;
>  | tarantool> 1,2,3,4
>  | 1, 2, 3, 4;
> 
> Fixes #4604
> 
> Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> issue: https://github.com/tarantool/tarantool/issues/4604
> branch: gorcunov/gh-4604-multireturn
> 
>  src/box/lua/console.lua | 38 +++++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index d4d8ec984..e9a9e8261 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -1,4 +1,6 @@
>  -- console.lua -- internal file
> +--
> +-- vim: ts=4 sw=4 et
>  
>  local serpent = require('serpent')
>  local internal = require('console')
> @@ -75,28 +77,38 @@ local serpent_map_symbols = function(tag, head, body, tail, level)
>      return tag..head..body..tail
>  end
>  
> -output_handlers["lua"] = function(status, opts, ...)
> -    --
> -    -- If no data present at least EOS should be put,
> -    -- otherwise wire readers won't be able to find
> -    -- where the end of string is.
> -    if not ... then
> -        return output_eos["lua"]
> -    end
> +--
> +-- Format element into lua form

Nit: no period at the end.

I would say that here we format a Lua value: element is quite common
word.

> +local function format_lua(opts, elem)

I would name the function format_lua_value() or like so.

`opts` is usually the last argument.

BTW, it is a bit unusual to use `opts` for a string value: is it worth
to rename it to `mode`?

>      for k,v in pairs(lua_map_direct_symbols) do
> -        if k == ... then
> -            return v .. output_eos["lua"]
> +        if k == elem then
> +            return v
>          end
>      end
>      local serpent_opts = {
>          custom  = serpent_map_symbols,
>          comment = false,
> -        nocode = true,
> +        nocode  = true,
>      }
>      if opts == "block" then
> -        return serpent.block(..., serpent_opts) .. output_eos["lua"]
> +        return serpent.block(elem, serpent_opts)
> +    end
> +    return serpent.line(elem, serpent_opts)
> +end
> +
> +output_handlers["lua"] = function(status, opts, ...)
> +    local collect = { }

Nit: we usually wrote `{}` w/o a whitespace.

> +    --
> +    -- If no data present at least EOS should be put,
> +    -- otherwise wire readers won't be able to find
> +    -- where the end of string is.
> +    if not ... then
> +        return output_eos["lua"]
> +    end
> +    for i = 1,select('#', ...) do

Nit: no whitespace after comma.

> +        collect[i] = format_lua(opts, select(i, ...))
>      end
> -    return serpent.line(..., serpent_opts) .. output_eos["lua"]
> +    return table.concat(collect, ', ') .. output_eos["lua"]
>  end
>  
>  local function output_verify_opts(fmt, opts)
> -- 
> 2.20.1
> 

Re: [Tarantool-patches] [PATCH] box/console: Handle multireturn in lua mode

[Cyrill Gorcunov]

On Wed, Dec 25, 2019 at 03:36:07PM +0300, Alexander Turenko wrote:
> Looks good in general and works correct even with nils at the end.
> 
> We however should add a test case for this, otherwise sooner or later
> we'll unable to make changes w/o fear to broke something. I propose to
> add a case with nils at the end as well as simple 1, 2, 3 multireturn.
> 
> I put several nits below. Some of them are pointed by our [Lua Style
> Guide][1], another ones are what I see across other parts of our
> codebase. My intention here is to keep the style more or less consistent
> and so make it more comfortable to work with the code (hope not only for
> me, otherwise it is not productive).
> 
> [1]: https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/

Thanks! I'll update and resend together with a testcase.