From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 9E31A469719 for ; Mon, 9 Nov 2020 17:38:58 +0300 (MSK) References: <6e71b54cc1c314746dfe9ff143bfcff6f9d0aeca.1603106269.git.artemreyt@tarantool.org> <20201023120931.GA17385@pony.bronevichok.ru> <4c35f2f2-901b-da2f-1ac5-312b59a300e1@tarantool.org> <20201105234659.dpwskwt5diulzphv@tkn_work_nb> From: Artem Message-ID: Date: Mon, 9 Nov 2020 17:38:57 +0300 MIME-Version: 1.0 In-Reply-To: <20201105234659.dpwskwt5diulzphv@tkn_work_nb> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: ru 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: Alexander Turenko , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org 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).