Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v7 2/2] test: fix luacheck warnings in test/sql-tap
Date: Fri, 15 Jan 2021 23:10:57 +0100	[thread overview]
Message-ID: <4c2cde7b-d6c1-2412-a067-361cd722d8c9@tarantool.org> (raw)
In-Reply-To: <ea6dcfbb906657ca842f85c85ed1da713c92c0d5.1610613097.git.sergeyb@tarantool.org>

Hi! Thanks for the patch!

> diff --git a/test/sql-tap/with2.test.lua b/test/sql-tap/with2.test.lua
> index 456ded363..cdf1d75a2 100755
> --- a/test/sql-tap/with2.test.lua
> +++ b/test/sql-tap/with2.test.lua
> @@ -386,22 +386,22 @@ genstmt(255), {
>  -- Check that adding a WITH clause to an INSERT disables the xfer
>  -- optimization.
>  
> -local function do_xfer_test(test, test_func, test_name, func, exp, opts)
> -    local opts = opts or {}
> +local function do_xfer_test(test_arg, test_func, test_name, func, exp, opts)
> +    opts = opts or {}
>      local exp_xfer_count = opts.exp_xfer_count
>      local before = box.stat.sql().sql_xfer_count
> -    test_func(test, test_name, func, exp)
> +    test_func(test_arg, test_name, func, exp)
>      local after = box.stat.sql().sql_xfer_count
>      test:is(after - before, exp_xfer_count,
>                     test_name .. '-xfer-count')
>  end
>  
> -test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
> -    do_xfer_test(test, test.do_execsql_test, test_name, func, exp, opts)
> +test.do_execsql_xfer_test = function(test_arg, test_name, func, exp, opts)
> +    do_xfer_test(test_arg, test_arg.do_execsql_test, test_name, func, exp, opts)
>  end
>  
> -test.do_catchsql_xfer_test = function(test, test_name, func, exp, opts)
> -    do_xfer_test(test, test.do_catchsql_test, test_name, func, exp, opts)
> +test.do_catchsql_xfer_test = function(test_arg, test_name, func, exp, opts)
> +    do_xfer_test(test_arg, test_arg.do_catchsql_test, test_name, func, exp, opts)
>  end
>  
>  test:do_execsql_test(

I reverted this entire diff hunk and luacheck passed.
Indeed, in this hunk you only fix upvalue redefinitions,
which we have agreed to leave as is, and you even added
ignores to .luacheckrc to the list of global ignores:

    -- Redefining a local variable.
    "411",
    -- Redefining an argument.
    "412",
    -- Shadowing a local variable.
    "421",
    -- Shadowing an upvalue.
    "431",
    -- Shadowing an upvalue argument.
    "432",

Why did you change this file?
Also I did this and the check also has passed:

diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
index 50300cdbe..b8955531a 100755
--- a/test/sql-tap/analyze9.test.lua
+++ b/test/sql-tap/analyze9.test.lua
@@ -244,7 +244,7 @@ test:do_execsql_test(
 local function insert_filler_rows_n(iStart, nCopy, nVal)
     for i = 0, nVal-1 do
         local iVal = iStart+i
-        for _ = 0, nCopy-1 do
+        for j = 0, nCopy-1 do
             box.execute(string.format("INSERT INTO t1 VALUES (null, %s, %s, '%s')", iVal, iVal, iVal))
         end
     end

I reverted one of your first changes in this commit.
The check passes because it looks like you left a ton of
ignores in files["test/sql-tap/**/*.lua"] in .luacheckrc.

These comments make me think that you checked the files
without using .luacheckrc config or I don't know.

I suggest you to re-make the patch with these ignores
and excess diff removed.

  reply	other threads:[~2021-01-15 22:11 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-14  8:52 [Tarantool-patches] [PATCH v7 0/2] Fix luacheck warnings in test/sql and test/sql-tap Sergey Bronnikov via Tarantool-patches
2021-01-14  8:52 ` [Tarantool-patches] [PATCH v7 1/2] test: fix luacheck warnings in test/sql Sergey Bronnikov via Tarantool-patches
2021-01-14  8:52 ` [Tarantool-patches] [PATCH v7 2/2] test: fix luacheck warnings in test/sql-tap Sergey Bronnikov via Tarantool-patches
2021-01-15 22:10   ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-01-21 12:52     ` Sergey Bronnikov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4c2cde7b-d6c1-2412-a067-361cd722d8c9@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v7 2/2] test: fix luacheck warnings in test/sql-tap' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox