From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v7] test: fix luacheck warnings in test/long_run-py Date: Wed, 13 Jan 2021 18:04:05 +0100 [thread overview] Message-ID: <1b302a98-cd94-bee3-ee7d-2e64f328bfde@tarantool.org> (raw) In-Reply-To: <c1c8611247bb175e17ad4d017960826d98e30bec.1610546460.git.sergeyb@tarantool.org> Thanks for the patch! See 3 comments below. On 13.01.2021 15:35, sergeyb@tarantool.org wrote: > From: Sergey Bronnikov <sergeyb@tarantool.org> > > Closes #5460 > > 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> > --- > > Changelog v7: > > - updated an exclusion mask in .luacheckrc > > Changelog v6: > > - splitted patch in test/ for patches per sub-directory > - adjusted supressions in .luacheckrc > - fixed formatting issues in .luacheckrc > > Gitlab CI: https://gitlab.com/tarantool/tarantool/-/pipelines/241108315 > Issue: https://github.com/tarantool/tarantool/issues/5460 > Branch: ligurio/gh-5460-luacheck-warnings-test-long_run-py > > .luacheckrc | 2 +- > test/long_run-py/lua/finalizers.lua | 8 +++----- > test/long_run-py/suite.lua | 16 +++++++++------- > 3 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/.luacheckrc b/.luacheckrc > index 4b829f3dc..68736d8db 100644 > --- a/.luacheckrc > +++ b/.luacheckrc > @@ -37,7 +37,7 @@ exclude_files = { > "test/box-tap/**/*.lua", > "test/engine/**/*.lua", > "test/engine_long/**/*.lua", > - "test/long_run-py/**/*.lua", > + "test/long_run-py/lua/finalizers.lua", 1. Why did you leave it in 'exclude' and yet you fix it? I tried to remove it, and found that it says your change in the next hunk is not correct. When I fixed it, I saw it says the 'result' variable is not used but only mutated. Then I tried to 'read' result somehow after the loop, but it says the code is unreachable. Then I made the loop look like it can end, and now it seems to be working. ==================== diff --git a/.luacheckrc b/.luacheckrc index 68736d8db..3f4913df3 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -37,7 +37,6 @@ exclude_files = { "test/box-tap/**/*.lua", "test/engine/**/*.lua", "test/engine_long/**/*.lua", - "test/long_run-py/lua/finalizers.lua", "test/luajit-tap/**/*.lua", "test/replication/**/*.lua", "test/replication-py/**/*.lua", diff --git a/test/long_run-py/lua/finalizers.lua b/test/long_run-py/lua/finalizers.lua index cb6400363..88bd7734c 100644 --- a/test/long_run-py/lua/finalizers.lua +++ b/test/long_run-py/lua/finalizers.lua @@ -7,10 +7,14 @@ local function test_finalizers() local result = {} local i = 1 local ffi = require('ffi') - while true do - local result[i] = ffi.gc(ffi.cast('void *', 0), on_gc) + while i ~= 0 do + result[i] = ffi.gc(ffi.cast('void *', 0), on_gc) i = i + 1 end + -- Fake-read 'result' to calm down 'luacheck' complaining that the variable + -- is never used. + assert(#result ~= 0) + return "done" end; test_finalizers() ==================== It looks a bit ugly maybe, because i ~= 0 is always true, and because there is unreachable code after the loop. I suggest to either find a better way to fix the warning; or don't change finalizers.lua at all and keep the whole file ignored; or apply my diff, or revert your 'local' usage in the next diff hunk, and silence this single warning about 'result' being not used. > "test/luajit-tap/**/*.lua", > "test/replication/**/*.lua", > "test/replication-py/**/*.lua", > diff --git a/test/long_run-py/lua/finalizers.lua b/test/long_run-py/lua/finalizers.lua > index 69146a323..cb6400363 100644 > --- a/test/long_run-py/lua/finalizers.lua > +++ b/test/long_run-py/lua/finalizers.lua > @@ -1,19 +1,17 @@ > #!/usr/bin/env tarantool > > -function on_gc(t) > +local function on_gc() > end; > > -function test_finalizers() > +local function test_finalizers() > local result = {} > local i = 1 > local ffi = require('ffi') > while true do > - result[i] = ffi.gc(ffi.cast('void *', 0), on_gc) > + local result[i] = ffi.gc(ffi.cast('void *', 0), on_gc) 2. This change is not correct. Even luacheck tells it, if you don't ignore this file. You assign a value to a table member, not declare a variable. > i = i + 1 > end > - return "done" > end; > > test_finalizers() > test_finalizers() > - > diff --git a/test/long_run-py/suite.lua b/test/long_run-py/suite.lua > index 0b33dec7d..7a09dd2b8 100644 > --- a/test/long_run-py/suite.lua > +++ b/test/long_run-py/suite.lua > @@ -109,3 +106,8 @@ function delete_insert(engine_name) > box.space.tester:drop() > return {counter, string_value_2} > end > + > +return { > + delete_replace_update = delete_replace_update; > + delete_insert = delete_insert; 3. Please, use ',' instead of ';'. > +} >
next prev parent reply other threads:[~2021-01-13 17:04 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-13 14:35 Sergey Bronnikov via Tarantool-patches 2021-01-13 17:04 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-01-14 8:13 ` Sergey Bronnikov via Tarantool-patches 2021-01-14 8:24 ` Sergey Bronnikov via Tarantool-patches 2021-01-14 21:46 ` Vladislav Shpilevoy via Tarantool-patches 2021-01-15 9:47 ` Sergey Bronnikov via Tarantool-patches 2021-01-15 22:20 ` Vladislav Shpilevoy via Tarantool-patches 2021-01-18 13:43 ` Kirill Yukhin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1b302a98-cd94-bee3-ee7d-2e64f328bfde@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=sergeyb@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v7] test: fix luacheck warnings in test/long_run-py' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox