From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 9881142F4C3 for ; Sat, 9 Nov 2019 03:39:25 +0300 (MSK) From: Alexander Turenko Date: Sat, 9 Nov 2019 03:39:16 +0300 Message-Id: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH] app/argparse: expect no value for a boolean option List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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:"]: Bad value for parameter "verh". Expected - boolean, got "42"' +- error: 'builtin/internal.argparse.lua:"]: Bad value for parameter "verh". No value + expected, got "42"' ... argparse({'--verh=42'}, {{'verh', 'boolean+'}}) --- -- error: 'builtin/internal.argparse.lua:"]: Bad value for parameter "verh". Expected - boolean, got "42"' +- error: 'builtin/internal.argparse.lua:"]: 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:"]: 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:"]: Bad value for parameter "foo". No value + | expected, got "bar"' + | ... + +params = {} + | --- + | ... +params[1] = {'foo', 'boolean+'} + | --- + | ... +argparse({'--foo=bar'}, params) + | --- + | - error: 'builtin/internal.argparse.lua:"]: 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