From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101])
 (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 E11BC469710
 for <tarantool-patches@dev.tarantool.org>;
 Mon,  1 Jun 2020 17:47:59 +0300 (MSK)
Date: Mon, 1 Jun 2020 17:47:41 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20200601144741.phdbyvvuyjwv6hjo@tkn_work_nb>
References: <cover.1590764167.git.sergeyb@tarantool.org>
 <b10824401496b58ef509ad3fc8b029071f88c8b6.1590764167.git.sergeyb@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <b10824401496b58ef509ad3fc8b029071f88c8b6.1590764167.git.sergeyb@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v6 05/25] Fix luacheck warnings in
	src/lua/
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: sergeyb@tarantool.org
Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org

> -local function parameters_parse(t_in, options)
> -    local t_out, t_in = {}, t_in or {}
> +local function parameters_parse(param, options)
> +    local t_out, t_in = {}, param or {}

Here you introduce the second name for one thing. It does not improve
the code readability...

>  
>      -- Prepare a lookup table for options. An option name -> a
>      -- type name to convert a parameter into or true (which means
> @@ -145,7 +145,7 @@ local function parameters_parse(t_in, options)
>                  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)
> +                    is_long, is_short, is_dash = parse_param_prefix(next_arg)

I don't see any reason to make the live interval of those very local
values longer. The code will be harder to verify.

I never see any positive impact from redefinitions detection in
luacheck, but those examples show that it may push us to introduce
different terms for one thing and to reuse one variable for logically
different values. It would be too pathetic to say that it is harmful,
but it certainly not good. (NB: Also said about this in [1].)

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017304.html

> -local function ibuf_tostring(ibuf)
> +local function ibuf_tostring()
>      return '<ibuf>'
>  end

The function contract assumes that it receives one argument. Whether
luacheck happy about this or not, we should not confuse a reader and
write the function like it does not receive any value.

I propose to use '-- luacheck: no unused args' for those cases, see [2].

[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017307.html

WBR, Alexander Turenko.