From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 467C5469710 for ; Mon, 1 Jun 2020 19:15:09 +0300 (MSK) Date: Mon, 1 Jun 2020 19:06:42 +0300 From: Igor Munkin Message-ID: <20200601160642.GS21558@tarantool.org> References: <8c80f5ce9c990027db258b1915d2e054a0fa90b3.1590764167.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <8c80f5ce9c990027db258b1915d2e054a0fa90b3.1590764167.git.sergeyb@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: sergeyb@tarantool.org Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, alexander.turenko@tarantool.org 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. 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. 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. > +} > +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. > 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. > 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. | -- 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. > 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