From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 AA6004429E1 for ; Sat, 20 Jun 2020 02:15:11 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <5010f149-6c50-9370-4394-930ed4771a0b@tarantool.org> Date: Sat, 20 Jun 2020 01:15:09 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org, imun@tarantool.org Cc: alexander.turenko@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 > 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? > "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 > 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. > 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.