From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 AB7BE469710 for ; Thu, 28 May 2020 18:17:04 +0300 (MSK) Date: Thu, 28 May 2020 18:16:06 +0300 From: Sergey Bronnikov Message-ID: <20200528151606.GA50538@pony.bronevichok.ru> References: <20200527165402.GA21558@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200527165402.GA21558@tarantool.org> 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: Igor Munkin Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Igor, thanks for review, see my comments inline. All changes are in branch. On 19:54 Wed 27 May , Igor Munkin wrote: > 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. Fixed. > > + 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. Fixed. > > + 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? It is not so, we use vanilla luacheck, look at the changes in .travis.mk. Anyway removed supressions for W122 and W142. > > +} > > files["src/lua/*.lua"] = {ignore = {"212/self"}} > > If unused self argument is suppressed source-wide, the suppression above > can be dropped. Agree, removed. > > 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. created issue [2] > > +files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}} > > The suppression above is excess since the issue is fixed via inline one. Removed. > > +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. Done in a whole .luacheckrc. > [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. Proposed fix brokes tests, so I just supressed warning inline. > > +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. Without 'tonumber64' in globals warning W113 triggered. See [1]. > 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 [1] https://gitlab.com/tarantool/tarantool/-/jobs/571452395 [2] https://github.com/tarantool/tarantool/issues/5032