From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 1CFCE469719 for ; Thu, 12 Nov 2020 15:04:50 +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> <20201110140945.GA55757@pony.bronevichok.ru> From: Artem Message-ID: <9578270f-95e3-734c-66fc-55eeed6e63c2@tarantool.org> Date: Thu, 12 Nov 2020 15:04:49 +0300 MIME-Version: 1.0 In-Reply-To: <20201110140945.GA55757@pony.bronevichok.ru> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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: Sergey Bronnikov , Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Sergey, see my 5 answers below. Branch is updated. 10.11.2020 17:09, Sergey Bronnikov пишет: > 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: 1. I explained the reason in commit message: "It was bad idea to add deep directories to `exclude_files`, because after that there is no way to enable files or child dirs of this directories for processing by luacheck. In this case, it was impossible to enable `test/sql-tap/` directory because whole `test/` was in `exclude_files` and consequently ignored." > 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. 2. See 3rd comment of my previous message (it doesn't work with `globals`). > --- 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 3. Removed > > 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: 4. Fixed in other cases too. > - 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? 5. It was my mistake, fixed. > 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).