Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

See 6 comments below.

I see on the branch there is now a mess with commits.
Here is how this commit looks on the branch.

====================
>     Fix luacheck warnings in extra/dist/
>     
>     Part of #4681
>     
>     Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
>     Reviewed-by: Igor Munkin <imun@tarantool.org>
>     
>     Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
>     Co-authored-by: Igor Munkin <imun@tarantool.org>
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index e6edf8617..90d6f7570 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -1,16 +1,51 @@
>  std = "luajit"
> +globals = {"box", "_TARANTOOL"}
> +ignore = {
> +    -- Unused argument <self>.
> +    "212/self",
> +    -- Redefining a local variable.
> +    "411",
> +    -- Shadowing an upvalue.
> +    "431",
> +}
>  
>  include_files = {
>      "**/*.lua",
> +    "extra/dist/tarantoolctl.in",
>  }
>  
>  exclude_files = {
>      "build/**/*.lua",
> -    "extra/**/*.lua",
> -    "src/**/*.lua",
> +    "src/box/**/*.lua",

1. Why did you enable src/lua check, but didn't fix src/lua warnings
in this commit? Instead, there is a next commit which actually fixes
the warnings. What makes this commit not atomic, related not only to
extra/dist/. While you said in the title

    "Fix luacheck warnings in extra/dist/".

This patch didn't touch src/lua before. Why did you change it?

>      "test-run/**/*.lua",
>      "test/**/*.lua",
>      "third_party/**/*.lua",
>      ".rocks/**/*.lua",
>      ".git/**/*.lua",
>  }
> +
> +files["extra/dist/tarantoolctl.in"] = {
> +    ignore = {
> +        -- https://github.com/tarantool/tarantool/issues/4929
> +        "122",
> +    },
> +}
> +files["src/lua/help.lua"] = {
> +    -- Globals defined for interactive mode.
> +    globals = {"help", "tutorial"},
> +}
> +files["src/lua/init.lua"] = {
> +    -- Miscellaneous global function definition.
> +    globals = {"dostring"},
> +    ignore = {
> +        -- Set tarantool specific behaviour for os.exit.
> +        "122/os",
> +        -- Add custom functions into Lua package namespace.
> +        "142/package",
> +    },
> +}
> +files["src/lua/swim.lua"] = {
> +    ignore = {
> +        "212/m", -- Respect swim module code style.

2. I thought we already dropped this exception and replaced m with self.
Why is it back?

> +    },
> +}

Now look at the next commit:

>     Fix luacheck warnings in src/lua/
>     
>     Part of #4681
>     
>     Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
>     Reviewed-by: Igor Munkin <imun@tarantool.org>
>     
>     Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
>     Co-authored-by: Igor Munkin <imun@tarantool.org>
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 90d6f7570..ef3d30857 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -1,12 +1,22 @@
>  std = "luajit"
> -globals = {"box", "_TARANTOOL"}
> +globals = {"box", "_TARANTOOL", "tonumber64"}
>  ignore = {
> +    -- Accessing an undefined field of a global variable <debug>.
> +    "143/debug",
> +    -- Accessing an undefined field of a global variable <string>.
> +    "143/string",
> +    -- Accessing an undefined field of a global variable <table>.
> +    "143/table",
>      -- Unused argument <self>.
>      "212/self",
>      -- Redefining a local variable.
>      "411",
> +    -- Shadowing a local variable.
> +    "421",
>      -- Shadowing an upvalue.
>      "431",
> +    -- Shadowing an upvalue argument.
> +    "432",
>  }
>  
>  include_files = {
> @@ -16,7 +26,8 @@ include_files = {
>  
>  exclude_files = {
>      "build/**/*.lua",
> -    "src/box/**/*.lua",
> +    -- Third-party source code.
> +    "src/box/lua/serpent.lua",

3. The commit says it fixes src/lua checks, but in fact

- src/lua checks were already enabled. Why were they enabled without
  having the warnings fixed?

- this commit enables src/box/lua checks. It does not seem
  related to src/lua things.

>      "test-run/**/*.lua",
>      "test/**/*.lua",
>      "third_party/**/*.lua",
> @@ -37,15 +48,10 @@ files["src/lua/help.lua"] = {
>  files["src/lua/init.lua"] = {
>      -- Miscellaneous global function definition.
>      globals = {"dostring"},
> -    ignore = {
> -        -- Set tarantool specific behaviour for os.exit.
> -        "122/os",
> -        -- Add custom functions into Lua package namespace.
> -        "142/package",
> -    },
>  }
> -files["src/lua/swim.lua"] = {
> +files["src/box/lua/console.lua"] = {
>      ignore = {
> -        "212/m", -- Respect swim module code style.
> -    },
> +        -- https://github.com/tarantool/tarantool/issues/5032
> +        "212",
> +    }
>  }

4. This hunk literally drops the code introduced in the previous
commit. Why did it change so radically? In the last review there
were just unnecessary diff hunks. And now the commits seem to be
completely screwed and mixed.

> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index bbdef01e3..a0e8faf38 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -341,7 +341,7 @@ local function do_wait(self, what, timeout)
>      local fd = check_socket(self)
>  
>      self._errno = nil
> -    timeout = timeout or TIMEOUT_INFINITY
> +    local timeout = timeout or TIMEOUT_INFINITY

5. You said you reverted it, but it is still here.

>  
>      repeat
>          local started = fiber.clock()
> @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
>          boxerrno(0)
>          return s
>      end
> -    local timeout = timeout or TIMEOUT_INFINITY
> +    timeout = timeout or TIMEOUT_INFINITY

6. Ditto.

I want to kindly ask you to spend more time on self-reviews
before sending a patch. Otherwise it consumes lots of time
when I need to point out the same things again and again.