From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D641246970E for ; Wed, 25 Dec 2019 15:36:07 +0300 (MSK) Date: Wed, 25 Dec 2019 15:36:07 +0300 From: Alexander Turenko Message-ID: <20191225123606.jnufsyn4exvryn2q@tkn_work_nb> References: <20191219174525.13826-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191219174525.13826-1-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH] box/console: Handle multireturn in lua mode List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml 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 > Signed-off-by: Cyrill Gorcunov > --- > 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 >