From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 6F39A469710 for ; Wed, 27 May 2020 20:02:27 +0300 (MSK) Date: Wed, 27 May 2020 19:54:02 +0300 From: Igor Munkin Message-ID: <20200527165402.GA21558@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v5 06/10] Fix luacheck warnings in src/box/lua/ List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy 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 > > Closes #4681 > > Reviewed-by: Vladislav Shpilevoy > Co-authored-by: Vladislav Shpilevoy > --- > .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 > -- > 2.23.0 > -- Best regards, IM