Re: [Tarantool-patches] [PATCH v5 06/10] Fix luacheck warnings in src/box/lua/

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch! There is a mess with suppressions (branch is OK),
so I just left only relevant part.

On 12.05.20, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Closes #4681
> 
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> ---
>  .luacheckrc | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 3cd05a254..e5c4c4509 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -29,6 +29,48 @@ files["extra/dist/tarantoolctl.in"] = {
>  	globals = {"box", "_TARANTOOL"},
>  	ignore = {"212/self", "122", "431"}
>  }
> +files["**/*.lua"] = {

Again, spaces vs tabs.

> +	globals = {"box", "_TARANTOOL", "help", "tutorial"},

OK, box and _TARANTOOL globals are widely used but AFAICS help and
tutorial occurs only in src/lua/help.lua and src/box/lua/console.lua, so
there is no need to suppress it source-wide.

> +	ignore = {"212/self", "122", "143", "142"}

I see no warning with [122] and [142] code, so they seem to be excess
here. Furthermore why do we need to suppress [143] globally if we don't
use vanilla luacheck?

> +}
>  files["src/lua/*.lua"] = {ignore = {"212/self"}}

If unused self argument is suppressed source-wide, the suppression above
can be dropped.

>  files["src/lua/init.lua"] = {globals = {"dostring"}}
>  files["src/lua/swim.lua"] = {ignore = {"431"}}
> +files["src/box/lua/console.lua"] = {ignore = {"212"}}

Well, why do we have so much unused args here? Can we fix such warnings?
Feel free to file a follow-up issue for this.

> +files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}

The suppression above is excess since the issue is fixed via inline one.

> +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "411"}}

One general comment: please sort suppressed codes, it's just more
convenient for further maintenance.

[411] warning can be fixed in a very simple way: just remove local prior
to create_transport on line 919 (consider the diff at the end). As a
result [411] suppression can be removed.

> +files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "432"}}

Why do we need to explicitly set tonumber64 as global and does not set
bit? Please choose one: either mention all globals used in sources to
prepare the sources to be checked against vanilla luacheck or just omit
such occurences if we're going to use our fork in CI.

As a result, after applying the following diff everything is OK:

================================================================================

diff --git a/.luacheckrc b/.luacheckrc
index a141e76ef..716f376b8 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -30,13 +30,11 @@ files["extra/dist/tarantoolctl.in"] = {
 	ignore = {"212/self", "122", "431"}
 }
 files["**/*.lua"] = {
-	globals = {"box", "_TARANTOOL", "help", "tutorial"},
-	ignore = {"212/self", "122", "143", "142"}
+    globals = {"box", "_TARANTOOL"},
+    ignore = {"212/self"},
 }
-files["src/lua/*.lua"] = {ignore = {"212/self"}}
 files["src/lua/init.lua"] = {globals = {"dostring"}}
 files["src/lua/swim.lua"] = {ignore = {"431"}}
-files["src/box/lua/console.lua"] = {ignore = {"212"}}
-files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
-files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "411"}}
-files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "432"}}
+files["src/box/lua/console.lua"] = {globals = {"help"}, ignore = {"212"}}
+files["src/box/lua/net_box.lua"] = {ignore = {"431", "432"}}
+files["src/box/lua/schema.lua"] = {ignore = {"431", "432"}}
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 3f2a9df81..646529173 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -916,7 +916,7 @@ end
 -- Now it is necessary to have a strong ref to callback somewhere or
 -- it is GC-ed prematurely. We wrap stop() method, stashing the
 -- ref in an upvalue (stop() performance doesn't matter much.)
-local create_transport = function(host, port, user, password, callback,
+create_transport = function(host, port, user, password, callback,
                                   connection, greeting)
     local weak_refs = setmetatable({callback = callback}, {__mode = 'v'})
     local function weak_callback(...)

================================================================================

I checked the src/box/lua/*.lua sources with our luacheck fork:
| $ tarantoolctl rocks install luacheck
| $ $ .rocks/bin/luacheck --codes --config .luacheckrc src/box/lua/*.lua
| Checking src/box/lua/console.lua                  OK
| Checking src/box/lua/feedback_daemon.lua          OK
| Checking src/box/lua/key_def.lua                  OK
| Checking src/box/lua/load_cfg.lua                 OK
| Checking src/box/lua/merger.lua                   OK
| Checking src/box/lua/net_box.lua                  OK
| Checking src/box/lua/schema.lua                   OK
| Checking src/box/lua/session.lua                  OK
| Checking src/box/lua/tuple.lua                    OK
| Checking src/box/lua/upgrade.lua                  OK
| Checking src/box/lua/xlog.lua                     OK
|
| Total: 0 warnings / 0 errors in 11 files

<snipped>

> -- 
> 2.23.0
> 

-- 
Best regards,
IM