Re: [Tarantool-patches] [PATCH v3 0/3] box/console: Fix multireturn and empty output

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

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 <alexander.turenko@tarantool.org>
 | Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
 | Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>

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
>