From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 E3393452566 for ; Tue, 12 Nov 2019 01:19:38 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <686ed898-91d1-546e-6120-ed7b1f46e1d7@tarantool.org> Date: Mon, 11 Nov 2019 23:25:44 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [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: Alexander Turenko Cc: tarantool-patches@dev.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)