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

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

[Alexander Turenko]

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:<line>"]: Bad value for parameter "verh". Expected
-    boolean, got "42"'
+- error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "verh". No value
+    expected, got "42"'
 ...
 argparse({'--verh=42'}, {{'verh', 'boolean+'}})
 ---
-- error: 'builtin/internal.argparse.lua:<line>"]: Bad value for parameter "verh". Expected
-    boolean, got "42"'
+- error: 'builtin/internal.argparse.lua:<line>"]: 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:<line>"]: 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:<line>"]: Bad value for parameter "foo". No value
+ |     expected, got "bar"'
+ | ...
+
+params = {}
+ | ---
+ | ...
+params[1] = {'foo', 'boolean+'}
+ | ---
+ | ...
+argparse({'--foo=bar'}, params)
+ | ---
+ | - error: 'builtin/internal.argparse.lua:<line>"]: 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

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

[Vladislav Shpilevoy]

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)

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

[Alexander Turenko]

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)

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

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

LGTM.

>>> +                -- 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.

There is no options. The first is the only standard in our code. If the
second one is used somewhere, then it is incorrect, or is a third-party
library with own code style.

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

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

[Alexander Turenko]

> >>> +                -- 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.
> 
> There is no options. The first is the only standard in our code. If the
> second one is used somewhere, then it is incorrect, or is a third-party
> library with own code style.

I see no mentions about this (at least there is nothing about this in
our Lua Style Guide on the website) as well as I see no dominant style
across our Lua code.

What is the source of your information?

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

[Vladislav Shpilevoy]

On 12/11/2019 22:03, Alexander Turenko wrote:
>>>>> +                -- 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.
>>
>> There is no options. The first is the only standard in our code. If the
>> second one is used somewhere, then it is incorrect, or is a third-party
>> library with own code style.
> 
> I see no mentions about this (at least there is nothing about this in
> our Lua Style Guide on the website) as well as I see no dominant style
> across our Lua code.
> 
> What is the source of your information?
> 

I remember, that I tried once to use a not aligned multiline 'if' and
was said/emailed by you, or Roman T., or Vladimir D., or by Kostja O.,
that in Lua we need to stick to the same rules as in C about alignment.

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

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/13 07:15]:
> >>> 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.
> >>
> >> There is no options. The first is the only standard in our code. If the
> >> second one is used somewhere, then it is incorrect, or is a third-party
> >> library with own code style.
> > 
> > I see no mentions about this (at least there is nothing about this in
> > our Lua Style Guide on the website) as well as I see no dominant style
> > across our Lua code.
> > 
> > What is the source of your information?
> > 
> 
> I remember, that I tried once to use a not aligned multiline 'if' and
> was said/emailed by you, or Roman T., or Vladimir D., or by Kostja O.,
> that in Lua we need to stick to the same rules as in C about alignment.

there is lua coding style that was added by Eugene. I believe it
is even online in the docs:

https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/

The code should stick to it.

-- 
Konstantin Osipov, Moscow, Russia

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

[Alexander Turenko]

On Wed, Nov 13, 2019 at 07:33:59AM +0300, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/13 07:15]:
> > >>> 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.
> > >>
> > >> There is no options. The first is the only standard in our code. If the
> > >> second one is used somewhere, then it is incorrect, or is a third-party
> > >> library with own code style.
> > > 
> > > I see no mentions about this (at least there is nothing about this in
> > > our Lua Style Guide on the website) as well as I see no dominant style
> > > across our Lua code.
> > > 
> > > What is the source of your information?
> > > 
> > 
> > I remember, that I tried once to use a not aligned multiline 'if' and
> > was said/emailed by you, or Roman T., or Vladimir D., or by Kostja O.,
> > that in Lua we need to stick to the same rules as in C about alignment.
> 
> there is lua coding style that was added by Eugene. I believe it
> is even online in the docs:
> 
> https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/
> 
> The code should stick to it.

It does not contain anything about a line continuation. One of three
linked style guides does, but it is a double indent.

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

[Alexander Turenko]

On Tue, Nov 12, 2019 at 09:41:45PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> LGTM.

We now looking how to better handle arguments for tarantoolctl rocks
(whitelisting, blacklisting, patching luarocks -- don't sure now), so it
is good to have this patch applied. (It is about #4629 and linked
issues.) CCed Leonid, he working on that.

Kirill said that he doesn't mind if I'll push the patch.

Pushed to master, 2.2, 2.1 and 1.10.

WBR, Alexander Turenko.