From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 9CE95469719 for ; Fri, 6 Nov 2020 02:46:58 +0300 (MSK) Date: Fri, 6 Nov 2020 02:46:59 +0300 From: Alexander Turenko Message-ID: <20201105234659.dpwskwt5diulzphv@tkn_work_nb> References: <6e71b54cc1c314746dfe9ff143bfcff6f9d0aeca.1603106269.git.artemreyt@tarantool.org> <20201023120931.GA17385@pony.bronevichok.ru> <4c35f2f2-901b-da2f-1ac5-312b59a300e1@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4c35f2f2-901b-da2f-1ac5-312b59a300e1@tarantool.org> 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 Thanks for the effort! 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. > +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

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: > #!/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).