[Tarantool-patches] [PATCH] luacheck: change global vars to local in sql-tap
Artem
artemreyt at tarantool.org
Mon Nov 9 17:38:57 MSK 2020
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