[Tarantool-patches] [PATCH] app/argparse: expect no value for a boolean option

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

Before commit 03f85d4cfffbd43dccef880525cb39e56e967ae1 ('app: fix
boolean handling in argparse module') the module does not expect a value
after a 'boolean' argument. However there was the problem: a 'boolean'
argument can be passed only at end of an argument list, otherwise it
wrongly consumes a next argument and gives a confusing error message.

The mentioned commit fixes this behaviour in the following way: it still
allows to pass a 'boolean' argument at end of the list w/o a value, but
requires a value ('true', 'false', '1', '0') if a 'boolean' argument is
not at the end to be provided using {'--foo=true'} or {'--foo', 'true'}
syntax.

Here this behaviour is changed: a 'boolean' argument does not assume an
explicitly passed value despite its position in an argument list. If a
'boolean' argument appears in the list, then argparse.parse() returns
`true` for its value (a list of `true` values in case of 'boolean+'
argument), otherwise it will not be added to the result.

This change also makes the behaviour of long (--foo) and short (-f)
'boolean' options consistent.

The motivation of the change is simple: it is easier and more natural to
type, say, `tarantoolctl cat --show-system 00000000000000000000.snap`
then `tarantoolctl cat --show-system true 00000000000000000000.snap`.

This commit adds several new test cases, but it does not mean that we
guarantee that the module behaviour will not be changed around some
corner cases, say, handling of 'boolean+' arguments. This is internal
module.

Fixes #4076 (again).
---

https://github.com/tarantool/tarantool/issues/4076
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4076-argparse-no-value-for-boolean-arg

 src/lua/argparse.lua                          |  59 +++++---
 test/app/argparse.result                      |   8 +-
 ...h-4076-argparse-wrong-bool-handling.result | 143 ++++++++++++++++--
 ...4076-argparse-wrong-bool-handling.test.lua |  60 +++++++-
 4 files changed, 226 insertions(+), 44 deletions(-)

diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
index 8dae388b1..45917ec0d 100644
--- a/src/lua/argparse.lua
+++ b/src/lua/argparse.lua
@@ -21,13 +21,18 @@ local function result_set_add(t_out, key, val)
 end
 
 local function err_bad_parameter_value(name, got, expected)
-    if type(got) ~= 'string' then
-        got = 'nothing'
+    assert(type(got) == 'boolean' or type(got) == 'string')
+    assert(type(got) ~= expected)
+
+    local reason
+    if type(got) == 'boolean' then
+        reason = ('Expected %s, got nothing'):format(expected)
+    elseif expected == 'boolean' or expected == 'boolean+' then
+        reason = ('No value expected, got "%s"'):format(got)
     else
-        got = string.format('"%s"', got)
+        reason = ('Expected %s, got "%s"'):format(expected, got)
     end
-    error(string.format('Bad value for parameter "%s". Expected %s, got %s',
-                        name, expected, got))
+    error(string.format('Bad value for parameter "%s". %s', name, reason))
 end
 
 local function convert_parameter_simple(name, convert_from, convert_to)
@@ -38,17 +43,9 @@ local function convert_parameter_simple(name, convert_from, convert_to)
         end
         return converted
     elseif convert_to == 'boolean' then
-        if type(convert_from) == 'boolean' then
-            return convert_from
-        end
-        convert_from = string.lower(convert_from)
-        if convert_from == '0' or convert_from == 'false' then
-            return false
-        end
-        if convert_from == '1' or convert_from == 'true' then
-            return true
+        if type(convert_from) ~= 'boolean' then
+            return err_bad_parameter_value(name, convert_from, convert_to)
         end
-        return err_bad_parameter_value(name, convert_from, convert_to)
     elseif convert_to == 'string' then
         if type(convert_from) ~= 'string' then
             return err_bad_parameter_value(name, convert_from, convert_to)
@@ -85,6 +82,20 @@ end
 
 local function parameters_parse(t_in, options)
     local t_out, t_in = {}, t_in or {}
+
+    -- Prepare a lookup table for options. An option name -> a
+    -- type name to convert a parameter into or true (which means
+    -- returning a value as is).
+    local lookup = {}
+    if options then
+        for _, v in ipairs(options) do
+            if type(v) ~= 'table' then
+                v = {v}
+            end
+            lookup[v[1]] = (v[2] or true)
+        end
+    end
+
     local skip_param = false
     for i, v in ipairs(t_in) do
         -- we've used this parameter as value
@@ -108,13 +119,21 @@ local function parameters_parse(t_in, options)
                 if key == nil or val == nil then
                     error(("bad argument #%d: ID not valid"):format(i))
                 end
+                -- Disallow an explicit value after '=' for a
+                -- 'boolean' or 'boolean+' argument.
+                if lookup[key] == 'boolean' or lookup[key] == 'boolean+' then
+                    return err_bad_parameter_value(key, val, lookup[key])
+                end
                 result_set_add(t_out, key, val)
             else
                 if command:match("^([%a_][%w_-]+)$") == nil then
                     error(("bad argument #%d: ID not valid"):format(i))
                 end
                 local val = true
-                do
+                -- Don't consume a value after a 'boolean' or
+                -- 'boolean+' argument.
+                if lookup[command] ~= 'boolean' and
+                        lookup[command] ~= 'boolean+' then
                     -- in case next argument is value of this key (not --arg)
                     local next_arg = t_in[i + 1]
                     local is_long, is_short, is_dash = parse_param_prefix(next_arg)
@@ -133,13 +152,7 @@ local function parameters_parse(t_in, options)
 ::nextparam::
     end
     if options then
-        local lookup, unknown = {}, {}
-        for _, v in ipairs(options) do
-            if type(v) ~= 'table' then
-                v = {v}
-            end
-            lookup[v[1]] = (v[2] or true)
-        end
+        local unknown = {}
         for k, v in pairs(t_out) do
             if lookup[k] == nil and type(k) == "string" then
                 table.insert(unknown, k)
diff --git a/test/app/argparse.result b/test/app/argparse.result
index 04f043999..f3a634e1d 100644
--- a/test/app/argparse.result
+++ b/test/app/argparse.result
@@ -137,13 +137,13 @@ argparse({'--verh=42'}, {'verh'})
 ...
 argparse({'--verh=42'}, {{'verh', 'boolean'}})
 ---
-- error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "verh". Expected
-    boolean, got "42"'
+- error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "verh". No value
+    expected, got "42"'
 ...
 argparse({'--verh=42'}, {{'verh', 'boolean+'}})
 ---
-- error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "verh". Expected
-    boolean, got "42"'
+- error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "verh". No value
+    expected, got "42"'
 ...
 argparse({'--verh=42'}, {'niz'})
 ---
diff --git a/test/app/gh-4076-argparse-wrong-bool-handling.result b/test/app/gh-4076-argparse-wrong-bool-handling.result
index fd933a37d..0abbc7bb7 100644
--- a/test/app/gh-4076-argparse-wrong-bool-handling.result
+++ b/test/app/gh-4076-argparse-wrong-bool-handling.result
@@ -25,31 +25,98 @@ params[2] = {'flag2', 'boolean'}
 params[3] = {'flag3', 'boolean'}
  | ---
  | ...
-params[4] = {'flag4', 'boolean'}
+args = {'--flag1', 'positional value', '--flag2'}
  | ---
  | ...
-params[5] = {'flag5', 'boolean'}
+argparse(args, params)
+ | ---
+ | - 1: positional value
+ |   flag1: true
+ |   flag2: true
+ | ...
+
+--
+-- When several 'boolean' arguments are passed, the result will be
+-- `true` (just as for one such argument).
+--
+params = {}
+ | ---
+ | ...
+params[1] = {'foo', 'boolean'}
  | ---
  | ...
-args = {'--flag1', 'true', '--flag2', '1', '--flag3', 'false', '--flag4', '0', '--flag5', 'TrUe'}
+args = {'--foo', '--foo'}
  | ---
  | ...
 argparse(args, params)
  | ---
- | - flag4: false
- |   flag1: true
- |   flag5: true
- |   flag2: true
- |   flag3: false
+ | - foo: true
  | ...
 
-args = {'--flag1', 'abc'}
+--
+-- When several 'boolean+' arguments are passed, the result will
+-- be a list of `true` values.
+--
+params = {}
+ | ---
+ | ...
+params[1] = {'foo', 'boolean+'}
+ | ---
+ | ...
+args = {'--foo', '--foo'}
  | ---
  | ...
 argparse(args, params)
  | ---
- | - error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "flag1". Expected
- |     boolean, got "abc"'
+ | - foo:
+ |   - true
+ |   - true
+ | ...
+
+params = {}
+ | ---
+ | ...
+params[1] = {'foo', 'boolean+'}
+ | ---
+ | ...
+args = {'--foo', 'positional value', '--foo'}
+ | ---
+ | ...
+argparse(args, params)
+ | ---
+ | - foo:
+ |   - true
+ |   - true
+ |   1: positional value
+ | ...
+
+--
+-- When a value is provided for a 'boolean' / 'boolean+' option
+-- using --foo=bar syntax, the error should state that a value is
+-- not expected for this option.
+--
+params = {}
+ | ---
+ | ...
+params[1] = {'foo', 'boolean'}
+ | ---
+ | ...
+argparse({'--foo=bar'}, params)
+ | ---
+ | - error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "foo". No value
+ |     expected, got "bar"'
+ | ...
+
+params = {}
+ | ---
+ | ...
+params[1] = {'foo', 'boolean+'}
+ | ---
+ | ...
+argparse({'--foo=bar'}, params)
+ | ---
+ | - error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "foo". No value
+ |     expected, got "bar"'
  | ...
 
 --
@@ -69,7 +136,10 @@ argparse({'--value'}, params)
  |     number, got nothing'
  | ...
 
-params[1][2] = 'string'
+params = {}
+ | ---
+ | ...
+params[1] = {'value', 'string'}
  | ---
  | ...
 argparse({'--value'}, params)
@@ -78,6 +148,55 @@ argparse({'--value'}, params)
  |     string, got nothing'
  | ...
 
+--
+-- Verify that short 'boolean' and 'boolean+' options behaviour
+-- is the same as for long options.
+--
+params = {}
+ | ---
+ | ...
+params[1] = {'f', 'boolean'}
+ | ---
+ | ...
+args = {'-f'}
+ | ---
+ | ...
+argparse(args, params)
+ | ---
+ | - f: true
+ | ...
+args = {'-f', '-f'}
+ | ---
+ | ...
+argparse(args, params)
+ | ---
+ | - f: true
+ | ...
+
+params = {}
+ | ---
+ | ...
+params[1] = {'f', 'boolean+'}
+ | ---
+ | ...
+args = {'-f'}
+ | ---
+ | ...
+argparse(args, params)
+ | ---
+ | - f:
+ |   - true
+ | ...
+args = {'-f', '-f'}
+ | ---
+ | ...
+argparse(args, params)
+ | ---
+ | - f:
+ |   - true
+ |   - true
+ | ...
+
 test_run:cmd("clear filter")
  | ---
  | - true
diff --git a/test/app/gh-4076-argparse-wrong-bool-handling.test.lua b/test/app/gh-4076-argparse-wrong-bool-handling.test.lua
index a8ee5e670..b4ce4c7e6 100644
--- a/test/app/gh-4076-argparse-wrong-bool-handling.test.lua
+++ b/test/app/gh-4076-argparse-wrong-bool-handling.test.lua
@@ -9,14 +9,45 @@ params = {}
 params[1] = {'flag1', 'boolean'}
 params[2] = {'flag2', 'boolean'}
 params[3] = {'flag3', 'boolean'}
-params[4] = {'flag4', 'boolean'}
-params[5] = {'flag5', 'boolean'}
-args = {'--flag1', 'true', '--flag2', '1', '--flag3', 'false', '--flag4', '0', '--flag5', 'TrUe'}
+args = {'--flag1', 'positional value', '--flag2'}
 argparse(args, params)
 
-args = {'--flag1', 'abc'}
+--
+-- When several 'boolean' arguments are passed, the result will be
+-- `true` (just as for one such argument).
+--
+params = {}
+params[1] = {'foo', 'boolean'}
+args = {'--foo', '--foo'}
+argparse(args, params)
+
+--
+-- When several 'boolean+' arguments are passed, the result will
+-- be a list of `true` values.
+--
+params = {}
+params[1] = {'foo', 'boolean+'}
+args = {'--foo', '--foo'}
+argparse(args, params)
+
+params = {}
+params[1] = {'foo', 'boolean+'}
+args = {'--foo', 'positional value', '--foo'}
 argparse(args, params)
 
+--
+-- When a value is provided for a 'boolean' / 'boolean+' option
+-- using --foo=bar syntax, the error should state that a value is
+-- not expected for this option.
+--
+params = {}
+params[1] = {'foo', 'boolean'}
+argparse({'--foo=bar'}, params)
+
+params = {}
+params[1] = {'foo', 'boolean+'}
+argparse({'--foo=bar'}, params)
+
 --
 -- When parameter value was omitted, it was replaced internally
 -- with boolean true, and sometimes was showed in error messages.
@@ -26,7 +57,26 @@ params = {}
 params[1] = {'value', 'number'}
 argparse({'--value'}, params)
 
-params[1][2] = 'string'
+params = {}
+params[1] = {'value', 'string'}
 argparse({'--value'}, params)
 
+--
+-- Verify that short 'boolean' and 'boolean+' options behaviour
+-- is the same as for long options.
+--
+params = {}
+params[1] = {'f', 'boolean'}
+args = {'-f'}
+argparse(args, params)
+args = {'-f', '-f'}
+argparse(args, params)
+
+params = {}
+params[1] = {'f', 'boolean+'}
+args = {'-f'}
+argparse(args, params)
+args = {'-f', '-f'}
+argparse(args, params)
+
 test_run:cmd("clear filter")
-- 
2.22.0