[Tarantool-patches] [PATCH] luacheck: change global vars to local in sql-tap
Artem
artemreyt at tarantool.org
Thu Nov 12 15:04:49 MSK 2020
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 <p && rm p
>>>> |
>>>> | # 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).
More information about the Tarantool-patches
mailing list