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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Thanks for review!

On 01:15 Sat 20 Jun , Vladislav Shpilevoy wrote:
> 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?

It is a mess due to inaccurate rebase. Fixed it in a branch.

> >      "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?

I see only one reason - due to inaccurate rebase.

> > +    },
> > +}
> 
> 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.

I have looked on patches again and made them atomic.
Each patch introduce only changes required by this patch.

> > 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.

reverted in a branch and pushed to remote:

local function do_wait(self, what, timeout)
    local fd = check_socket(self)

    self._errno = nil
    timeout = timeout or TIMEOUT_INFINITY

    repeat

> >  
> >      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.

        return s
    end
    local timeout = timeout or TIMEOUT_INFINITY
    local stop = fiber.clock() + timeout
    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
        protocol = 'tcp' })
    if dns == nil or #dns == 0 then
        boxerrno(boxerrno.EINVAL)
        return nil

> 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.

This time I spend more time on self-review before sending updated patches.  I
have applied changes suggested by you in a comments, self-reviewed changes in
each patch, run "luacheck ." after applying each patch.