From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 B0A14445320 for ; Thu, 16 Jul 2020 16:23:47 +0300 (MSK) Date: Thu, 16 Jul 2020 16:23:45 +0300 From: Sergey Bronnikov Message-ID: <20200716132345.GB8696@pony.bronevichok.ru> References: <8c80f5ce9c990027db258b1915d2e054a0fa90b3.1590764167.git.sergeyb@tarantool.org> <20200601160642.GS21558@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200601160642.GS21558@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v6 09/25] Fix luacheck warnings in test/box 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, alexander.turenko@tarantool.org Igor, On 19:06 Mon 01 Jun , Igor Munkin wrote: > Sergey, > > Thanks for the patch! > > Again, I see a mess in the branch. Please review yourself before sending > the patchset: mixed tabs and spaces, unsorted enries in .luacheckrc -- > we are talking about it since I joined the review (i.e. v3 series). Now > it's a v6 and I definitely don't want another one, but I doubt we will > avoid it. Furthermore the changes you made in scope of this patch just > partially fix the problem. Yes, I see you committed/rebased some > follow-up patches on the branch few hours ago and it's another > consequence of omitting self-review. Hasty fixes are not necessary (I > have another patchesets to be reviewed and my own tasks), but pointing > to the same mistakes just makes me frustrating. It would be great if you > re-check yourself once more before running git send-email next time. > This will definitely make the review process much easier, efficient and > ergo faster. > > Please send follow-up patches to this thread, since it's too hard to > comment the changes you made after the series was sent. > > I reviewed this one and its follow-up you recently pushed. Please > consider the comments below. > > On 29.05.20, sergeyb@tarantool.org wrote: > > From: Sergey Bronnikov > > > > Part of #4681 > > > > Reviewed-by: Vladislav Shpilevoy > > Reviewed-by: Igor Munkin > > > > Co-authored-by: Vladislav Shpilevoy > > Co-authored-by: Igor Munkin > > --- > > .luacheckrc | 13 +++++++++++++ > > test/box/box.lua | 2 +- > > test/box/hash_multipart.result | 2 +- > > test/box/hash_multipart.test.lua | 2 +- > > test/box/lua/bitset.lua | 7 ++----- > > test/box/lua/cfg_bad_vinyl_dir.lua | 2 +- > > test/box/lua/cfg_rtree.lua | 2 +- > > test/box/lua/cfg_test1.lua | 2 +- > > test/box/lua/cfg_test2.lua | 2 +- > > test/box/lua/cfg_test3.lua | 2 +- > > test/box/lua/cfg_test4.lua | 2 +- > > test/box/lua/cfg_test5.lua | 4 ++-- > > test/box/lua/cfg_test6.lua | 2 +- > > test/box/lua/fifo.lua | 2 +- > > test/box/lua/identifier.lua | 9 ++++----- > > test/box/lua/index_random_test.lua | 2 +- > > test/box/lua/require_init.lua | 3 --- > > test/box/lua/test_init.lua | 10 +++++----- > > test/box/lua/utils.lua | 13 ++++++------- > > test/box/on_schema_init.lua | 2 +- > > test/box/proxy.lua | 2 +- > > test/box/tiny.lua | 2 +- > > test/box/tree_pk.result | 4 ++-- > > test/box/tree_pk.test.lua | 4 ++-- > > 24 files changed, 51 insertions(+), 46 deletions(-) > > > > diff --git a/.luacheckrc b/.luacheckrc > > index 630834e16..f99cc17eb 100644 > > --- a/.luacheckrc > > +++ b/.luacheckrc > > @@ -41,3 +41,16 @@ files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "4 > > files["test/app/lua/fiber.lua"] = {globals = {"box_fiber_run_test"}} > > files["test/app-tap/lua/require_mod.lua"] = {globals = {"exports"}} > > files["test/app-tap/string.test.lua"] = {globals = {"utf8"}} > > +files["test/box/lua/push.lua"] = {globals = {"push_collection"}} > > +files["test/box/lua/index_random_test.lua"] = {globals = {"index_random_test"}} > > +files["test/box/lua/utils.lua"] = { > > + globals = {"space_field_types", "iterate", "arithmetic", "table_shuffle", > > + "table_generate", "tuple_to_string", "check_space", "space_bsize", > > + "create_iterator", "setmap", "sort"}} > > +files["test/box/lua/bitset.lua"] = { > > + globals = {"create_space", "fill", "delete", "clear", "drop_space", > > + "dump", "test_insert_delete"} > > One more time: please sort the entries. It *really* helps for reviewing > and further maintenance. I don't want to refer the last time I was > asking it, please just fix. I have splitted lines per variable and sorted alphabetically. Update is in a branch and a new patch series. > One more time: please adjust the indentation (tabs vs spaces) and to use > only one as we already discussed several times. I don't want to refer > the last time I was asking it, please just fix. Fixed too. > Furthermore, I can't figure out the rules you're using for indentation. > .luacheckrc is kinda Lua source file, so please adjust it regarding our > contrubition guide. The result should be similar to the example below: > | files["test/box/lua/bitset.lua"] = { > | globals = { > | "create_space", "fill", "delete", "clear", "drop_space", > | "dump", "test_insert_delete" > | } > | } > > Feel free to put each global entry to a seraparate line if it seems > better for you. As said above I have put each entry to a separate line. > > +} > > +files["test/box/lua/fifo.lua"] = {globals = {"fifomax", "find_or_create_fifo", "fifo_push", "fifo_top"}} > > +files["test/box/lua/identifier.lua"] = {globals = {"run_test"}} > > +files["test/box/lua/require_mod.lua"] = {globals = {"exports"}} > > > > I see no rule for checking .result files, *but* I know the exact reason, > why I see such changes in the patch. The mask ought to be updated *prior > to or together with* the changes. Please remove this change, since > test/box/hash_miltipart.test.lua is a diff-based test, thereby has to be > skipped by luacheck as we discussed several times. reviewed overall patch series and removed all changes for diff-based tests > > diff --git a/test/box/hash_multipart.result b/test/box/hash_multipart.result > > index e94313b62..e6915652f 100644 > > --- a/test/box/hash_multipart.result > > +++ b/test/box/hash_multipart.result > > @@ -62,7 +62,7 @@ test_run:cmd("setopt delimiter ';'") > > function select_all() > > local result = {} > > local tuple, v > > - for tuple, v in hash:pairs() do > > + for _, v in hash:pairs() do > > table.insert(result, v) > > end > > return result > > diff --git a/test/box/hash_multipart.test.lua b/test/box/hash_multipart.test.lua > > index c0a871bee..c572d13b6 100644 > > --- a/test/box/hash_multipart.test.lua > > +++ b/test/box/hash_multipart.test.lua > > @@ -24,7 +24,7 @@ test_run:cmd("setopt delimiter ';'") > > function select_all() > > local result = {} > > local tuple, v > > - for tuple, v in hash:pairs() do > > + for _, v in hash:pairs() do > > table.insert(result, v) > > end > > return result > > > > I failed to find any source that uses require_init.lua, require_mod.lua > (except require_init.lua) and test_init.lua. Looks these chunk can be > just dropped. But I'm totally against such actions in scope of this > patch, so please revert the changes related to these files, add them to > the exclude list like you did for test_init.lua in the follow-up patch > and create an issue for further actions. Well, marked them to be dropped: "test/app/*.test.lua", - "test/box/**/*.lua", + "test/box/*.test.lua", + -- Unused source file, to be dropped (gh-5169). + "test/box/lua/require_init.lua", + -- Unused source file, to be dropped (gh-5169). + "test/box/lua/require_mod.lua", + -- Unused source file, to be dropped (gh-5169). + "test/box/lua/test_init.lua", "test/box-py/**/*.lua", > > diff --git a/test/box/lua/require_init.lua b/test/box/lua/require_init.lua > > > > > diff --git a/test/box/lua/test_init.lua b/test/box/lua/test_init.lua > > > > > diff --git a/test/box/on_schema_init.lua b/test/box/on_schema_init.lua > > index 17cf89166..f74d6d7fe 100644 > > --- a/test/box/on_schema_init.lua > > +++ b/test/box/on_schema_init.lua > > Here is the diff from the follow-up patch: > > | diff --git a/test/box/on_schema_init.lua b/test/box/on_schema_init.lua > | index f74d6d7fe..a32470b40 100644 > | --- a/test/box/on_schema_init.lua > | +++ b/test/box/on_schema_init.lua > | @@ -1,12 +1,12 @@ > | #!/usr/bin/env tarantool > | local os = require('os') > | > | -function test_before_replace_trig(old, new) > | +local function test_before_replace_trig(old, new) -- luacheck: ignore > > I see no reason to use "ignore" here and below instead of "no unused > args" that explicitly describes the suppressed warning. I have set exact ignore reason in a luacheck comment > | -- return multiple values so that the stack fills earlier. > | return new:update{{'+', 2, 1}}, new:update{{'+', 2, 1}}, new:update{{'+', 2, 1}}, new:update{{'+', 2, 1}} > | end > | > | -function space_on_replace_trig(old, new) > | +local function space_on_replace_trig(old, new) -- luacheck: ignore > | if new and new[3] == 'test_on_schema_init' then > | box.on_commit(function() > | box.space.test_on_schema_init:before_replace(test_before_replace_trig) > | @@ -14,7 +14,7 @@ function space_on_replace_trig(old, new) > | end > | end > | > | -function on_init_trig() > | +local function on_init_trig() > | box.space._space:on_replace(space_on_replace_trig) > | end > | > > > > Again, the changeset below is made for diff-based tests, please drop it. Done. > > diff --git a/test/box/tree_pk.result b/test/box/tree_pk.result > > index df3c78bed..f2a4f5352 100644 > > --- a/test/box/tree_pk.result > > +++ b/test/box/tree_pk.result > > @@ -153,8 +153,8 @@ test_run:cmd("setopt delimiter ';'") > > ... > > function crossjoin(space0, space1, limit) > > local result = {} > > - for state, v0 in space0:pairs() do > > - for state, v1 in space1:pairs() do > > + for _, v0 in space0:pairs() do > > + for _, v1 in space1:pairs() do > > if limit <= 0 then > > return result > > end > > diff --git a/test/box/tree_pk.test.lua b/test/box/tree_pk.test.lua > > index 1190ab424..86041a642 100644 > > --- a/test/box/tree_pk.test.lua > > +++ b/test/box/tree_pk.test.lua > > @@ -58,8 +58,8 @@ test_run = env.new() > > test_run:cmd("setopt delimiter ';'") > > function crossjoin(space0, space1, limit) > > local result = {} > > - for state, v0 in space0:pairs() do > > - for state, v1 in space1:pairs() do > > + for _, v0 in space0:pairs() do > > + for _, v1 in space1:pairs() do > > if limit <= 0 then > > return result > > end > > -- > > 2.23.0 > > > > -- > Best regards, > IM