[Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
Sergey Bronnikov
sergeyb at tarantool.org
Mon Jul 13 13:01:28 MSK 2020
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 at tarantool.org>
> > Reviewed-by: Igor Munkin <imun at tarantool.org>
> >
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > Co-authored-by: Igor Munkin <imun at 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 at tarantool.org>
> > Reviewed-by: Igor Munkin <imun at tarantool.org>
> >
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > Co-authored-by: Igor Munkin <imun at 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.
More information about the Tarantool-patches
mailing list