From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 55B93440F3C for ; Tue, 12 Nov 2019 20:40:50 +0300 (MSK) Date: Tue, 12 Nov 2019 20:40:47 +0300 From: Alexander Turenko Message-ID: <20191112174047.tirafxzpyiytkxfl@tkn_work_nb> References: <686ed898-91d1-546e-6120-ed7b1f46e1d7@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <686ed898-91d1-546e-6120-ed7b1f46e1d7@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Updated the patch. See the incremental diff below. WBR, Alexander Turenko. > > Fixes #4076 (again). > > 1. Strictly speaking, it is not a fix, it is a follow-up. The bug is > fixed already. Closed the issue back, marked the commit as the follow up. > > --- > > > > https://github.com/tarantool/tarantool/issues/4076 > > https://github.com/tarantool/tarantool/tree/Totktonada/gh-4076-argparse-no-value-for-boolean-arg > > + -- 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. Let's consider three variants of splitting long if conditions: | if long_cond1 or | long_cond2 then | <...> | end | if long_cond1 or | long_cond2 then | <...> | end | if long_cond1 or | long_cond2 then | <...> | end As I see the first and the second variants are used across tarantool's built-in Lua code. You are right, third one, which I personally prefer, is not used within the project. Maybe it is better to always wrap such conditions into a function and assign to a variable. Anyway, now it is wrapped into a function. > 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. It is hard to choose a name here. I stopped on parameter_has_value(). > > -- 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) ---- diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua index 45917ec0d..faa0ae130 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) @@ -27,7 +37,7 @@ local function err_bad_parameter_value(name, got, expected) local reason if type(got) == 'boolean' then reason = ('Expected %s, got nothing'):format(expected) - elseif expected == 'boolean' or expected == 'boolean+' then + elseif not parameter_has_value(expected) then reason = ('No value expected, got "%s"'):format(got) else reason = ('Expected %s, got "%s"'):format(expected, got) @@ -121,7 +131,7 @@ local function parameters_parse(t_in, options) end -- Disallow an explicit value after '=' for a -- 'boolean' or 'boolean+' argument. - if lookup[key] == 'boolean' or lookup[key] == 'boolean+' then + 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) @@ -132,8 +142,7 @@ local function parameters_parse(t_in, options) local val = true -- Don't consume a value after a 'boolean' or -- 'boolean+' argument. - if lookup[command] ~= 'boolean' and - lookup[command] ~= 'boolean+' then + 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)