Re: [Tarantool-patches] [PATCH v4 4/10] Fix luacheck warnings in src/lua/

[Sergey Bronnikov <sergeyb@tarantool.org>]

Igor,

On 17:13 Thu 23 Apr , Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! I left several comments below, please consider
> them.
> 
> On 21.04.20, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>

<snipped>

> >  files["extra/dist/tarantoolctl.in"] = {ignore = {"212/self", "122", "431"}}
> > +files["src/lua/swim.lua"] = {ignore = {"431"}}
> > +files["src/lua/fio.lua"] = {ignore = {"231"}}
> > +files["src/lua/init.lua"] = {globals = {"dostring"}}
> 
> There are several "(W212) unused argument self" warnings for crypto.lua
> (and other chunks like digest.lua, errno.lua, etc), but no corresponding
> suppression in .luacheckrc like the one you added for tarantoolctl.in in
> the previous patch.
>
> Furthermore I see no reason to leave suppressions list unsorted.

Added supression 212/self for file mask "src/lua/*.lua" and sorted
list. Changes pushed to a branch.

<snipped>

> | > > -local function parameters_parse(t_in, options)
> | > > -    local t_out, t_in = {}, t_in or {}
> | > > +local function parameters_parse(t__in, options)
> | > > +    local t_out, t_in = {}, t__in or {}
> | >
> | > I guess you can just give t__in a proper name, e.g. args, params, etc.
> |
> | Agree with you, replaced it with 'param'.
> |
> 
> I see no corresponding changes in v4.

Applied again and updated in a branch.

<snipped>

> This was also mentioned in the v3 series review[2]:
> | This value is unused (and the corresponding warning is reported).
> | Consider the changes below:
> |
> | ================================================================================
> |
> | diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> | index b2b4b4c77..e271eccf6 100644
> | --- a/src/lua/fio.lua
> | +++ b/src/lua/fio.lua
> | @@ -407,7 +407,6 @@ fio.rmtree = function(path)
> |      if type(path) ~= 'string' then
> |          error("Usage: fio.rmtree(path)")
> |      end
> | -    local status
> |      path = fio.abspath(path)
> |      local ls, err = fio.listdir(path)
> |      if err ~= nil then
> | @@ -427,7 +426,8 @@ fio.rmtree = function(path)
> |              end
> |          end
> |      end
> | -    status, err = fio.rmdir(path)
> | +    local _
> | +    _, err = fio.rmdir(path)
> |      if err ~= nil then
> |          return false, string.format("failed to remove %s: %s", path, err)
> |      end
> |
> | ================================================================================
> 
> I see no reason to suppress it in the .luacheckrc instead of fixing a
> single occurense.

Fixed and pushed to a branch too.

<snipped>

> > --- a/src/lua/help.lua
> > +++ b/src/lua/help.lua
> 
> <snipped>
> 
> > @@ -22,7 +19,7 @@ setmetatable(help, { __call = help_call })
> >  
> >  local screen_id = 1;
> >  
> > -local function tutorial_call(table, action)
> > +local function tutorial_call(action)
> 
> This change breaks the behaviour. Consider the following:
> | $ ./src/tarantool
> | Tarantool 2.4.0-192-g89f3b427d
> | type 'help' for interactive help
> | tarantool> tutorial('start')
> | ---
> | - error: 'builtin/help.lua:32: Usage: tutorial("start" | "next" | "prev" | 1 .. 20)'
> | ...
> |
> | tarantool> getmetatable(tutorial).__call('start')
> | ---
> | - |
> |   Tutorial -- Screen #1 -- Hello, Moon
> |   ====================================
> |
> |   Welcome to the Tarantool tutorial.
> |   It will introduce you to Tarantool’s Lua application server
> |   and database server, which is what’s running what you’re seeing.
> |   This is INTERACTIVE -- you’re expected to enter requests
> |   based on the suggestions or examples in the screen’s text.
> |
> |   The first request is:
> |
> |   5.1, "Olá", "Lua"
> |   ------------------
> |
> |   This isn’t your grandfather’s "Hello World" ...
> |   the decimal literal 5.1 and the two strings inside
> |   single quotes ("Hello Moon" in Portuguese) will just
> |   be repeated, without need for a print() statement.
> |
> |   Take that one-line request and enter it below after the
> |   "tarantool>" prompt, then type Enter.
> |   You’ll see the response:
> |   ---
> |   - 5.1
> |   - Olá
> |   - Lua
> |   ...
> |   Then you’ll get a chance to repeat -- perhaps entering
> |   something else such as "Longer String",-1,-3,0.
> |   When you’re ready to go to the next screen, enter <tutorial("next")>.
> | ...
> |
> | tarantool>
> 
> Long story short: table arg is similar to self here (see more here[3])
> and cannot be simply dropped. Nevertheless it looks more convenient to
> replace it with self.

replaced 'table' argument by 'self'. tutorial('start') works as exprected.

> I guess it would be nice to add a test for it (or at least file a
> follow-up issue requesting the test).

https://github.com/tarantool/tarantool/issues/4930

<snipped>