From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 F1FA44696C3 for ; Fri, 24 Apr 2020 12:13:04 +0300 (MSK) Date: Fri, 24 Apr 2020 12:12:59 +0300 From: Sergey Bronnikov Message-ID: <20200424091259.GC24166@pony.bronevichok.ru> References: <89f3b427d1ccf4a85fab38c30d5ea23497f3a709.1587476678.git.sergeyb@tarantool.org> <20200423141326.GB11314@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200423141326.GB11314@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 4/10] Fix luacheck warnings in src/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, v.shpilevoy@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 > > 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. > | > > -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. > 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. > > --- a/src/lua/help.lua > > +++ b/src/lua/help.lua > > > > > @@ -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 . > | ... > | > | 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