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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 3 comments below.

On 09/11/2019 01:39, Alexander Turenko wrote:
> 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).

1. Strictly speaking, it is not a fix, it is a follow-up. The bug is
fixed already.

> ---
> 
> 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(-)
> 
> @@ -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

2. Something is wrong with the indentation.

3. This check "type ~= boolean and type ~= boolean+" is repeated 3
times in this file now. I would add a function like 'type_has_value(type)'
or similar which would encapsulate it.

>                      -- 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)