[Tarantool-patches] [PATCH] luacheck: change global vars to local in sql-tap
Sergey Bronnikov
sergeyb at tarantool.org
Tue Nov 10 17:09:45 MSK 2020
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:
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.
--- 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
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:
- 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?
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).
--
sergeyb@
More information about the Tarantool-patches
mailing list