Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] app/argparse: expect no value for a boolean option
Date: Mon, 11 Nov 2019 23:25:44 +0100	[thread overview]
Message-ID: <686ed898-91d1-546e-6120-ed7b1f46e1d7@tarantool.org> (raw)
In-Reply-To: <c5ad5ef9e4aecd36d9feb3bb076ceae401d3e1db.1573259560.git.alexander.turenko@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)

  reply	other threads:[~2019-11-11 22:19 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-09  0:39 Alexander Turenko
2019-11-11 22:25 ` Vladislav Shpilevoy [this message]
2019-11-12 17:40   ` Alexander Turenko
2019-11-12 20:41     ` Vladislav Shpilevoy
2019-11-12 21:03       ` Alexander Turenko
2019-11-12 21:33         ` Vladislav Shpilevoy
2019-11-13  4:33           ` Konstantin Osipov
2019-11-13  8:40             ` Alexander Turenko
2019-11-15  0:18       ` Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=686ed898-91d1-546e-6120-ed7b1f46e1d7@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] app/argparse: expect no value for a boolean option' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox