From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 D18D6469719 for ; Tue, 10 Nov 2020 17:09:46 +0300 (MSK) Date: Tue, 10 Nov 2020 17:09:45 +0300 From: Sergey Bronnikov Message-ID: <20201110140945.GA55757@pony.bronevichok.ru> References: <6e71b54cc1c314746dfe9ff143bfcff6f9d0aeca.1603106269.git.artemreyt@tarantool.org> <20201023120931.GA17385@pony.bronevichok.ru> <4c35f2f2-901b-da2f-1ac5-312b59a300e1@tarantool.org> <20201105234659.dpwskwt5diulzphv@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH] luacheck: change global vars to local in sql-tap List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Artem Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko Artem, see my 5 comments below. --- a/.luacheckrc +++ b/.luacheckrc @@ -22,25 +22,60 @@ ignore = { } 1. I don't get it why we should replace blacklist of files to whitelist. And there is unneeded change in config: files["src/lua/help.lua"] = { -- Globals defined for interactive mode. globals = {"help", "tutorial"}, } + files["src/lua/init.lua"] = { -- Miscellaneous global function definition. globals = {"dostring"}, --- a/test/sql-tap/alias.test.lua +++ b/test/sql-tap/alias.test.lua @@ -1,5 +1,5 @@ #!/usr/bin/env tarantool -test = require("sqltester") +local test = require("sqltester") test:plan(9) --!./tcltestrunner.lua @@ -24,6 +24,7 @@ test:plan(9) -- A procedure to return a sequence of increasing integers. -- +-- luacheck: globals counter counter = 0 2. We decided to keep supression lines on the same line. --- a/test/sql-tap/analyzeF.test.lua +++ b/test/sql-tap/analyzeF.test.lua @@ -1,8 +1,8 @@ -where_clauses_x = {"x = 4 AND y = 19", "x = '4' AND y = '19'", +local where_clauses_x = {"x = 4 AND y = 19", "x = '4' AND y = '19'", x = substr('145', 2, 1) AND y = substr('5195', 2, 2)"} -where_clauses_y = {"x = 19 AND y = 4", "x = '19' AND y = '4'", +local where_clauses_y = {"x = 19 AND y = 4", "x = '19' AND y = '4'", 3. trailing spaces added 4. in a previous review I asked you to reduce some changes. For example in a file test/sql-tap/analyze5.test.lua you can easily reduce change to 2-lines change: - x = z - w = z - t = (z + 0.5) + local x = z + local w = z + local t = (z + 0.5) + local u if z == 0 then to reduced patch: + local x, w, t x = z w = z t = (z + 0.5) + local u if z == 0 then In the same file: function() - w2 = v[1]:gsub('y', '+y'):gsub('z', '+z') - a1 = test:execsql("SELECT id FROM t1 NOT INDEXED WHERE "..w2.." ORDER BY +id") - a2 = test:execsql("SELECT id FROM t1 WHERE "..v[1].." ORDER BY +id") + local w2 = v[1]:gsub('y', '+y'):gsub('z', '+z') + local a1 = test:execsql("SELECT id FROM t1 NOT INDEXED WHERE "..w2.." ORDER BY +id") + local a2 = test:execsql("SELECT id FROM t1 WHERE "..v[1].." ORDER BY +id") + local res if (test.is_deeply_regex(a1, a2)) Why have you ignore comment? 5. What are the reasons of these changes? b/test/sql-tap/e_expr.test.lua - local A = val[2] - local B = val[3] - local C = val[4] - local testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn) + local A, B, C, testname + A = val[2] + B = val[3] + C = val[4] + testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn) On 17:38 Mon 09 Nov , Artem wrote: > Thanks for your comments! > > Pushed fixes on https://github.com/tarantool/tarantool/tree/artemreyt/gh-5173-fix-luacheck-warnings-sql-tap > > Commented your notes below. > > 06.11.2020 02:46, Alexander Turenko writes: > > Thanks for the effort! > 1. I fixed those warnings, which were produced be `luacheck .`, now it's OK. > > Just `luacheck .` shows the following warnings, for example: > > > > test/sql-tap/minmax2.test.lua:64:9: setting non-standard global variable sql_search_count > > test/sql-tap/subquery.test.lua:696:9: setting non-standard global variable callcnt > > > > It seems, `make luacheck` target does not work. > > > > | $ make VERBOSE=1 luacheck > > | <...> > > | /usr/bin/luacheck --codes --config \ > > | /home/alex/p/tarantool-meta/r/t-3/.luacheckrc \ > > | /home/alex/p/tarantool-meta/r/t-3 > > | <...> > > > > If I run luacheck this way, it does not give any warnings. This does not > > look as the problem from your patch, but it is something we should fix > > prior it. > 2. Changed whitelist to blacklist and referred to gh-5464 here. > > > +files["test/sql-tap/**/*.lua"] = { > > > + only = { > > > + -- Setting an undefined global variable. > > > + "111" > > > + } > > > +} > > > > > > Can we mention further issues here (gh-5181 and gh-5464)? It would make > > > clear that this clause is necessary only as the temporary solution until > > > we'll actually fix more / all warnings. I'll be glad if you'll add a > > > list of warnings and its amount into the gh-5464 issue: it'll help with > > > futher estimation. > > > > > > Can we use a blacklist instead of the whitelist? This way we'll also > > > catch all standard warnings except the listed ones. It seems, there are > > > not so many warnings to blacklist: > > > > > > | $ luacheck --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \ > > > | grep -o '(W[0-9]\+)' | sort -u | cat -n > > > | 1 (W111) > > > | 2 (W113) > > > | 3 (W211) > > > | 4 (W212) > > > | 5 (W213) > > > | 6 (W231) > > > | 7 (W311) > > > | 8 (W511) > > > | 9 (W512) > > > | 10 (W542) > > > | 11 (W611) > > > | 12 (W612) > > > | 13 (W613) > > > | 14 (W614) > > > | 15 (W621) > > > | 16 (W631) > > > > > > I would ask you to comment each of them (like you do it for the > > > whitelist) and leave an issue numbers, where it is expected to be fixed. > > > > > > Some warnings that were not appear before, appear now: > > > > > > | # Remove the suppression > > > | $ git diff | cat > > > | diff --git a/.luacheckrc b/.luacheckrc > > > | index 6fbd1a5b3..e9a906f08 100644 > > > | --- a/.luacheckrc > > > | +++ b/.luacheckrc > > > | @@ -33,13 +33,6 @@ exclude_files = { > > > | "src/box/lua/serpent.lua", > > > | } > > > | > > > | -files["test/sql-tap/**/*.lua"] = { > > > | - only = { > > > | - -- Setting an undefined global variable. > > > | - "111" > > > | - } > > > | -} > > > | - > > > | files["src/lua/help.lua"] = { > > > | -- Globals defined for interactive mode. > > > | globals = {"help", "tutorial"}, > > > | > > > | # Revert the changes in the test/ directory. > > > | $ git show HEAD test >p && patch -R -p1

> > | > > > | # Collect warnings. > > > | $ luacheck --no-color --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \ > > > | grep -o '(W[0-9]\+).*$' | sort | uniq -c | sort -rnk1,1 >before.txt > > > | > > > | # Apply back the changes in the test/ directory. > > > | $ git checkout -- test > > > | > > > | # Collect the warnings again. > > > | $ luacheck --no-color --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \ > > > | grep -o '(W[0-9]\+).*$' | sort | uniq -c | sort -rnk1,1 >after.txt > > > | > > > | # Look at the changed counters. > > > | $ diff -u before.txt after.txt | grep '^+' > > > | +++ after.txt 2020-11-06 01:40:41.940542423 +0300 > > > | + 58 (W211) unused variable 'testprefix' > > > | + 33 (W631) line is too long (132 > 120) > > > | + 20 (W631) line is too long (126 > 120) > > > | + 9 (W113) accessing undefined variable 'sql_search_count' > > > | + 9 (W111) setting non-standard global variable 'sql_search_count' > > > | + 8 (W113) accessing undefined variable 'sql' > > > | + 7 (W211) unused variable 'i' > > > | + 6 (W111) setting non-standard global variable 'counter' > > > | + 4 (W211) unused variable 'json' > > > | + 4 (W113) accessing undefined variable 'i' > > > | + 3 (W211) unused variable 'yaml' > > > | + 3 (W211) unused variable 's' > > > | + 2 (W211) unused variable 'rc' > > > | + 2 (W211) unused variable 'msg' > > > | + 2 (W113) accessing undefined variable 'STMT' > > > | + 2 (W113) accessing undefined variable 'res' > > > | + 2 (W111) setting non-standard global variable 'callcnt' > > > | + 1 (W231) variable 'V' is never accessed > > > | + 1 (W231) variable 'c' is never accessed > > > | + 1 (W231) variable 'b' is never accessed > > > | + 1 (W231) variable 'a' is never accessed > > > | + 1 (W211) unused variable 'view_v1' > > > | + 1 (W211) unused variable 'sql_search_count' > > > | + 1 (W211) unused variable 'prev' > > > | + 1 (W211) unused variable 'NULL' > > > | + 1 (W211) unused variable 'nm' > > > | + 1 (W211) unused variable 'limit' > > > | + 1 (W211) unused variable 'i0' > > > | + 1 (W211) unused variable 'fio' > > > | + 1 (W211) unused variable 'DB' > > > | + 1 (W113) accessing undefined variable 'y' > > > | + 1 (W113) accessing undefined variable 'stmt' > > > | + 1 (W113) accessing undefined variable 'r' > > > | + 1 (W113) accessing undefined variable 'DB' > > > | > > > | # Look more thoroughly. > > > | $ diff -u before.txt after.txt | grep '^[-+]' > > > > > > Some of those new warnings looks suspectful, e.g. 'testprefix' should be > > > removed in several places instead of making it local. We should verify > > > all those cases. I would formulate the criteria as follows: a fix of a > > > warning should not produce any new warnings (even such as 'too long > > > line'). > > > > > > test/sql-tap/e_expr.test.lua: > > > > > > - local A = val[2] > > > - local B = val[3] > > > - local C = val[4] > > > - local testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn) > > > + local A, B, C, testname > > > + A = val[2] > > > + B = val[3] > > > + C = val[4] > > > + testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn) > > It seems, you changed those lines unintentionally. > > > > test/sql-tap/gh2250-trigger-chain-limit.test.lua: > > > > > - function check(sql) > > > - msg = '' > > > +local function check(sql) > > > + local msg = '' > > The indentation of the 'local function' clause is broken. > > > > test/sql-tap/index7.test.lua: > > > > > -- Capture the output of a pragma in a TEMP table. > > > -- > > > -local function capture_pragma(db, tabname, sql) > > > +local function capture_pragma(db, tabname, sql) -- luacheck: ignore once > > > once = 1 > > > end > > The function is referred only from the commented code. There is no any > > reference to 'once' too. In fact, it is a kind of inaccurate converting > > from the original sqlite test ([1]). I would just remove it. If we'll > > implement partial indices and will enable related SQL tests, we'll > > anyway need to look into the original code. > > > > [1]: https://sqlite.org/src/file?name=test/index7.test > > > > test/sql-tap/trigger9.test.lua: > > 3. Unfortunately, `globals` inline option doesn't work with variables one > the same line, > > so i was forced to use two lines in such places. > > > > #!/usr/bin/env tarantool > > > +-- luacheck: globals test > > > test = require("sqltester") > > > test:plan(28) > > I all other places you add the luacheck directive on the same line as a > > corresponding Lua statement. Let's do this consistently. > > > > I'm okay with doing it in two lines if it becomes too long when written > > in one line (not this case). -- sergeyb@