[tarantool-patches] [PATCH 0/3] box/console: Cleanup and bug fix

[tarantool-patches] [PATCH 0/3] box/console: Cleanup and bug fix

[Cyrill Gorcunov]

In first two patches simply make code smaller and in
third patch fix a real bug I catched when started to
investigate test migration to lua output.

---

The following changes since commit f21a9932293bbbb5f3b6a3c6fa69a5bb3953e4b1:

  LTO build fails on warning message (2019-07-26 20:52:43 +0300)

are available in the Git repository at:

  git@github.com:cyrillos/tarantool.git console-repl-serpent-5

for you to fetch changes up to dfdfb6d9a67427c39b6f8403c111c61078f68909:

  box/console: Test for nil value in args directly (2019-07-27 01:13:00 +0300)

----------------------------------------------------------------
Cyrill Gorcunov (3):
      box/console: Don't allow arguments in get_default_output
      box/console: Drop redundant status parameter from return
      box/console: Test for nil value in args directly

 src/box/lua/console.lua | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)
-- 
2.20.1

[tarantool-patches] [PATCH 1/3] box/console: Don't allow arguments in get_default_output

[Cyrill Gorcunov]

The function

 | require('console').get_default_output()

requires no arguments. Make it explcicit and print
an error otherwise.

Part-of #3834
---
 src/box/lua/console.lua | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index d4f61865c..9258e7bbb 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -115,7 +115,11 @@ local function set_default_output(value)
     default_output_format["opts"] = opts
 end
 
-local function get_default_output()
+local function get_default_output(...)
+    local args = ...
+    if args ~= nil then
+        error("Arguments provided while prohibited")
+    end
     return default_output_format
 end
 
-- 
2.20.1

[tarantool-patches] [PATCH 2/3] box/console: Drop redundant status parameter from return

[Cyrill Gorcunov]

In output_verify_opts and output_parse we return status variable
to point if function processed without error. This is redundant
we can simply return either error or nil, which is enough.

Part-of #3834
---
 src/box/lua/console.lua | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 9258e7bbb..0c521b44a 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -72,15 +72,15 @@ end
 
 local function output_verify_opts(fmt, opts)
     if opts == nil then
-        return true, nil
+        return nil
     end
     if fmt == "lua" then
         if opts ~= "line" and opts ~= "block" then
             local msg = 'Wrong option "%s", expecting: line or block.'
-            return false, msg:format(opts)
+            return msg:format(opts)
         end
     end
-    return true, nil
+    return nil
 end
 
 local function output_parse(value)
@@ -92,23 +92,23 @@ local function output_parse(value)
     end
     for k, _ in pairs(output_handlers) do
         if k == fmt then
-            local status, err = output_verify_opts(fmt, opts)
-            if status ~= true then
-                return false, err
+            local err = output_verify_opts(fmt, opts)
+            if err then
+                return err
             end
-            return true, nil, fmt, opts
+            return nil, fmt, opts
         end
     end
     local msg = 'Invalid format "%s", supported languages: lua and yaml.'
-    return false, msg:format(value)
+    return msg:format(value)
 end
 
 local function set_default_output(value)
     if value == nil then
         error("Nil output value passed")
     end
-    local status, err, fmt, opts = output_parse(value)
-    if status ~= true then
+    local err, fmt, opts = output_parse(value)
+    if err then
         error(err)
     end
     default_output_format["fmt"] = fmt
@@ -180,8 +180,8 @@ local function set_language(storage, value)
 end
 
 local function set_output(storage, value)
-    local status, err, fmt, opts = output_parse(value)
-    if status ~= true then
+    local err, fmt, opts = output_parse(value)
+    if err then
         return error(err)
     end
     output_save(fmt, opts)
-- 
2.20.1

[tarantool-patches] [PATCH 3/3] box/console: Test for nil value in args directly

[Cyrill Gorcunov]

Instead of allocating a variable for optional args
testing we should use dot notation instead. Otherwise
it won't work for trivial test case as

```
require('console').set_default_output('lua,block')
require('decimal').new('1234.5678')
```

 | builtin/box/console.lua:47: expected decimal, number or string as 2 argument

and program exits.

Part-of #3834
---
 src/box/lua/console.lua | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 0c521b44a..64086cf5b 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -41,10 +41,9 @@ output_handlers["yaml"] = function(status, opts, ...)
 end
 
 output_handlers["lua"] = function(status, opts, ...)
-    local data = ...
     --
     -- Don't print nil if there is no data
-    if data == nil then
+    if not ... then
         return ""
     end
     --
-- 
2.20.1

[tarantool-patches] Re: [PATCH 1/3] box/console: Don't allow arguments in get_default_output

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/07/27 01:21]:
> -local function get_default_output()
> +local function get_default_output(...)
> +    local args = ...

I think it's enough to check for a single argument, variadic
arguments introduce unnecessary overhead.

> +    if args ~= nil then
> +        error("Arguments provided while prohibited")

error("This function accepts no arguments")
> +    end
>      return default_output_format
>  end

Please add a test case.
Otherwise lgtm.

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 2/3] box/console: Drop redundant status parameter from return

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/07/27 01:21]:
> In output_verify_opts and output_parse we return status variable
> to point if function processed without error. This is redundant
> we can simply return either error or nil, which is enough.

lgtm.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] box/console: Test for nil value in args directly

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/07/27 01:21]:

> Instead of allocating a variable for optional args
> testing we should use dot notation instead. Otherwise
> it won't work for trivial test case as

Please remove extra "instead"

> 
> ```
> require('console').set_default_output('lua,block')
> require('decimal').new('1234.5678')
> ```
> 
>  | builtin/box/console.lua:47: expected decimal, number or string as 2 argument
> 
> and program exits.
> 
> Part-of #3834
> ---
>  src/box/lua/console.lua | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index 0c521b44a..64086cf5b 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -41,10 +41,9 @@ output_handlers["yaml"] = function(status, opts, ...)
>  end
>  
>  output_handlers["lua"] = function(status, opts, ...)
> -    local data = ...
>      --
>      -- Don't print nil if there is no data
> -    if data == nil then
> +    if not ... then
>          return ""
>      end
>      --

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] box/console: Test for nil value in args directly

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/07/27 01:21]:
> Instead of allocating a variable for optional args
> testing we should use dot notation instead. Otherwise
> it won't work for trivial test case as
> 
> ```
> require('console').set_default_output('lua,block')
> require('decimal').new('1234.5678')

Actually, could you add a test? then lgtm :)

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] box/console: Test for nil value in args directly

[Cyrill Gorcunov]

On Sun, Jul 28, 2019 at 09:47:02PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/07/27 01:21]:
> > Instead of allocating a variable for optional args
> > testing we should use dot notation instead. Otherwise
> > it won't work for trivial test case as
> > 
> > ```
> > require('console').set_default_output('lua,block')
> > require('decimal').new('1234.5678')
> 
> Actually, could you add a test? then lgtm :)

Yes, tests are my first priority now. I've been reading test
engine code (the process is not that fast as I thought of)
and occasionally spotted the problem. Anyway I'll rework
the series and resend together with test. Thanks for review!

[tarantool-patches] Re: [PATCH 1/3] box/console: Don't allow arguments in get_default_output

[Cyrill Gorcunov]

On Sun, Jul 28, 2019 at 09:44:22PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/07/27 01:21]:
> > -local function get_default_output()
> > +local function get_default_output(...)
> > +    local args = ...
> 
> I think it's enough to check for a single argument, variadic
> arguments introduce unnecessary overhead.

Good point, thanks!

> > +    if args ~= nil then
> > +        error("Arguments provided while prohibited")
> 
> error("This function accepts no arguments")

OK

> > +    end
> >      return default_output_format
> >  end
> 
> Please add a test case.
> Otherwise lgtm.

Will rework and resend.

[tarantool-patches] Re: [PATCH 0/3] box/console: Cleanup and bug fix

[Kirill Yukhin]

Hello,

On 27 Jul 01:17, Cyrill Gorcunov wrote:
> In first two patches simply make code smaller and in
> third patch fix a real bug I catched when started to
> investigate test migration to lua output.
> 
> ---
> 
> The following changes since commit f21a9932293bbbb5f3b6a3c6fa69a5bb3953e4b1:
> 
>   LTO build fails on warning message (2019-07-26 20:52:43 +0300)
> 
> are available in the Git repository at:
> 
>   git@github.com:cyrillos/tarantool.git console-repl-serpent-5
> 
> for you to fetch changes up to dfdfb6d9a67427c39b6f8403c111c61078f68909:
> 
>   box/console: Test for nil value in args directly (2019-07-27 01:13:00 +0300)
> 
> ----------------------------------------------------------------
> Cyrill Gorcunov (3):
>       box/console: Don't allow arguments in get_default_output
>       box/console: Drop redundant status parameter from return
>       box/console: Test for nil value in args directly

The patchset is pretty much important from stability point of view. So,
checked it into master.

Cyrill, I still hope you'll prepare tests.

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH 0/3] box/console: Cleanup and bug fix

[Cyrill Gorcunov]

On Fri, Aug 02, 2019 at 03:59:26PM +0300, Kirill Yukhin wrote:
> The patchset is pretty much important from stability point of view. So,
> checked it into master.
> 
> Cyrill, I still hope you'll prepare tests.

Yes, patches are most priority for me now.