[Tarantool-patches] [PATCH v4 4/10] Fix luacheck warnings in src/lua/
Sergey Bronnikov
sergeyb at tarantool.org
Fri Apr 24 12:12:59 MSK 2020
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 at tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb at 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>
More information about the Tarantool-patches
mailing list