Re: [Tarantool-patches] [PATCH] luacheck: change global vars to local in sql-tap

[Artem <artemreyt@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 <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).