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 D267546970E for ; Tue, 21 Jan 2020 18:16:36 +0300 (MSK) Date: Tue, 21 Jan 2020 18:16:42 +0300 From: Alexander Turenko Message-ID: <20200121151641.cv2ka3t7cewbjsau@tkn_work_nb> References: <20200120175950.28060-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200120175950.28060-1-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v3 0/3] box/console: Fix multireturn and empty output List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml Thanks! There are several missed comments. Cited from [1]: > > From previous review (see [1]): > > > I propose to add a case with nils at the end as well as simple 1, 2, 3 > > multireturn. > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013326.html > > I meant '1, nil, nil, nil\n' or kinda. Your implementation (after the patch) > works correctly: it does not cut trailing nils. I don't sure whether it is part > of our contract, to be honest. But while we're tentative it is good to keep > this property and add the test case. Added: diff --git a/test/app-tap/console_lua.test.lua b/test/app-tap/console_lua.test.lua index 5848bdf69..3ed6aad97 100755 --- a/test/app-tap/console_lua.test.lua +++ b/test/app-tap/console_lua.test.lua @@ -111,6 +111,16 @@ local cases = { input = '1, 2, nil, a', expected = '1, 2, box.NULL, {\n 1,\n 2,\n 3\n}', cleanup = 'a = nil', + }, { + name = 'trailing nils, line mode', + opts = {block = false}, + input = '1, nil, nil, nil', + expected = '1, box.NULL, box.NULL, box.NULL', + }, { + name = 'trailing nils, block mode', + opts = {block = true}, + input = '1, nil, nil, nil', + expected = '1, box.NULL, box.NULL, box.NULL', }, { name = 'empty output', input = '\\set output', Cited from [2]: > > test/app-tap: add console_lua test > > Nit: we often use 'test:' prefix and it is useful for searching / > excluding of such commits. I would use it here too. > > > > > Test multireturn and empty output in lua mode. > > Nit: I would say "lack of a parameter of '\set output' command" or at > least "empty output format", because 'empty output' is hard to > understand w/o looking into previous commits. Changed to: | test: add app-tap/console_lua test | | Test multireturn in lua output mode and lack of a parameter in '\set | output <...>' command. | | Co-developed-by: Alexander Turenko | Signed-off-by: Cyrill Gorcunov | Reviewed-by: Alexander Turenko I don't think that a second review is necessary for those patches, they are simple. Pushed to master, 2.3, 2.2. CCed Kirill. [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013595.html [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013672.html WBR, Alexander Turenko. On Mon, Jan 20, 2020 at 08:59:47PM +0300, Cyrill Gorcunov wrote: > In v3 nits for tests are fixed. > > The issues are > > https://github.com/tarantool/tarantool/issues/4604 > https://github.com/tarantool/tarantool/issues/4638 > > Branch > > gorcunov/console-fixes-3 > > Cyrill Gorcunov (3): > box/console: handle multireturn in lua mode > box/console: handle empty output format > test/app-tap: add console_lua test > > src/box/lua/console.lua | 41 ++++++--- > test/app-tap/console_lua.test.lua | 141 ++++++++++++++++++++++++++++++ > 2 files changed, 169 insertions(+), 13 deletions(-) > create mode 100755 test/app-tap/console_lua.test.lua > > -- > 2.20.1 >