From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 D973E445320 for ; Mon, 13 Jul 2020 13:01:29 +0300 (MSK) Date: Mon, 13 Jul 2020 13:01:28 +0300 From: Sergey Bronnikov Message-ID: <20200713100128.GB2944@pony.bronevichok.ru> References: <5010f149-6c50-9370-4394-930ed4771a0b@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5010f149-6c50-9370-4394-930ed4771a0b@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/ List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: alexander.turenko@tarantool.org, tarantool-patches@dev.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 > > Reviewed-by: Igor Munkin > > > > Co-authored-by: Vladislav Shpilevoy > > Co-authored-by: Igor Munkin > > > > 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 . > > + "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 > > Reviewed-by: Igor Munkin > > > > Co-authored-by: Vladislav Shpilevoy > > Co-authored-by: Igor Munkin > > > > 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 . > > + "143/debug", > > + -- Accessing an undefined field of a global variable . > > + "143/string", > > + -- Accessing an undefined field of a global variable . > > + "143/table", > > -- Unused argument . > > "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.