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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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
>