From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0B81345C304 for ; Wed, 9 Dec 2020 02:30:51 +0300 (MSK) Date: Wed, 9 Dec 2020 02:30:56 +0300 From: Alexander Turenko Message-ID: <20201208233056.imqc7agpaqd7obem@tkn_work_nb> References: <9fa9abe33284ca30177a0642fdd042b5155578e9.1605889896.git.artemreyt@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <9fa9abe33284ca30177a0642fdd042b5155578e9.1605889896.git.artemreyt@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2] luacheck: change global vars to local in sql-tap List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Artem Starshov Cc: tarantool-patches@dev.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.