From 1167534795db477ef408d5035c4bb16887d18320 Mon Sep 17 00:00:00 2001
From: Alexander Turenko <alexander.turenko@tarantool.org>
Date: Wed, 6 Nov 2019 20:36:16 +0300
Subject: [PATCH] app/argparse: expect no value for a boolean option
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.
Follows up #4076.
Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
(cherry picked from commit e47f2c91612cc0396b89dd7eebd8d3b83e33e7ed)
---
src/lua/argparse.lua | 68 ++++++---
test/app/argparse.result | 8 +-
...h-4076-argparse-wrong-bool-handling.result | 143 ++++++++++++++++--
...4076-argparse-wrong-bool-handling.test.lua | 60 +++++++-
4 files changed, 235 insertions(+), 44 deletions(-)
diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
index 8dae388b1e..faa0ae1306 100644
--- a/src/lua/argparse.lua
+++ b/src/lua/argparse.lua
@@ -8,6 +8,16 @@ local function parse_param_prefix(param)
return is_long, is_short, is_dash
end
+-- Determine whether a value should be provided for a parameter.
+--
+-- The value can be passed after '=' within the same argument or
+-- in a next argument.
+--
+-- @param convert_to a type of the parameter
+local function parameter_has_value(convert_to)
+ return convert_to ~= 'boolean' and convert_to ~= 'boolean+'
+end
+
local function result_set_add(t_out, key, val)
if val == nil then
table.insert(t_out, key)
@@ -21,13 +31,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 not parameter_has_value(expected) 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 +53,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 +92,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 +129,20 @@ 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 not parameter_has_value(lookup[key]) 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 parameter_has_value(lookup[command]) 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 +161,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 04f0439996..f3a634e1d7 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 fd933a37dc..0abbc7bb76 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 a8ee5e6703..b4ce4c7e68 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")
--
GitLab