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

[Alexander Turenko <alexander.turenko@tarantool.org>]

Sorry for the late response.

LGTM with several optional comments. No need to re-review with me if
you'll decide to implement the suggestions.

Please, rebase it on top of the latest master and ask approve from the
QA team (I suggest to add Alexander V. Tikhonov to CC).

See comments inline.

WBR, Alexander Turenko.

> diff --git a/test/sql-tap/alias.test.lua b/test/sql-tap/alias.test.lua
> index 75391b305..8181b5ed5 100755
> --- 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

To be honest, I would prefer an explicit global variable:

 | _G.counter = 0

(And the same for all next usages.)

It gives a hint for a reader, not only suppresses the warning.

The same can be applied for a function declarations. Where we have:

 | foo = function(x, y, z)
 |     <...>
 | end

or

 | function foo(x, y, z)
 |     <...>
 | end

We can write:

 | _G.foo = function(x, y, z)
 |     <...>
 | end

or, better,

 | local function foo(x, y, z)
 |     <...>
 | end
 | _G.foo = foo

But call it as usual: foo() -- without explicit _G.

I'm too late here, so I'll leave it up to you.

> diff --git a/test/sql-tap/analyzeF.test.lua b/test/sql-tap/analyzeF.test.lua
> index 8f6fb5c97..e56437902 100755
> --- a/test/sql-tap/analyzeF.test.lua
> +++ b/test/sql-tap/analyzeF.test.lua
> <...>
> -where_clause_x = {"x = det4() AND y = det19()"}
> +where_clauses_x = {"x = det4() AND y = det19()"}
>  where_clauses_y = {"x = det19() AND y = det4()"}

Nice catch!

Sadly, the test is disabled and becomes broken due to replacing of
box.internal.sql_create_function() with box.schema.func.create(). We
unable to verify whether the change will break the test. Anyway, it is
out of scope of this task; just interesting.

> diff --git a/test/sql-tap/gh2548-select-compound-limit.test.lua b/test/sql-tap/gh2548-select-compound-limit.test.lua
> index 76769d6ae..30cf63939 100755
> --- a/test/sql-tap/gh2548-select-compound-limit.test.lua
> +++ b/test/sql-tap/gh2548-select-compound-limit.test.lua
> <...>
> -select_string_last = ''
> +local select_string_last = ''
>  
> <...>
>  
> -    select_string_last = select_string
> +    select_string_last = select_string -- luacheck: globals select_string_last

It is not a global variable. This suppression is redundant.

> --- a/test/sql-tap/index1.test.lua
> +++ b/test/sql-tap/index1.test.lua
> <...>
> @@ -1011,6 +1011,7 @@ end
>  test:do_test(
>      "index-22.1.0",
>      function()
> +        local format, s
>          format = {}
>          format[1] = { name = 'id', type = 'scalar'}
>          format[2] = { name = 'f2', type = 'scalar'}

's' (the result of the create_space() call) is not used anywhere, so can
be just removed. I mean, the result of create_space() may be not saved
anywhere.

The same for 'i' in lua-tables.test.lua and 's' in the same file in the
'no-format-create-index-prep' subtest.

The same for 's0' and 'i0' in sql-errors.test.lua.

The same for 'i' in sql-tap/alter.test.lua ('alter-2.3.prepare') and 's'
in the 'alter-8.1.0' subtest in the same file.

The same for 'i' in delete1.test.lua.

We'll need to fix it anyway, when we'll enable the warning re unused
values.