Re: [Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib*

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Igor,

On 16.02.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the review!
> 
> On 14.02.21, Sergey Kaplun wrote:
> > Hi, Igor!
> > 
> > Thanks for the patch!
> > 
> > Looks like I was late for the ball and you got two LGTMs, but here’s my
> 
> No, you were not!
> 
> > two cents on the matter, because I've been mentioned. Feel free to
> > ignore my nitpicks.
> > 
> > LGTM, except two nitpicks below.
> 
> Added your tag:
> | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

Thanks!

> 
> > 
> > On 02.02.21, Igor Munkin wrote:
> > > Since the regular static analysis has not been enabled for the test
> > > chunks in LuaJIT repository yet, the tests for recently implemented
> > > features still produce luacheck warnings.
> > > 
> > > The most of the issues are fixed in scope of the commit
> > > 8fc103fb1a21c28185a1942e75d8d9485e3aade7 ('test: fix warnings spotted by
> > > luacheck') and this patch fixes the remaining ones.
> > > 
> > > Fixes tarantool/tarantool#5631
> > 
> > Nit: Looks like 'Closes' or even 'Part of' (considering the next patch),
> > not 'Fixes'.
> 
> Why? Sergey pointed out the warnings. The sources and the related
> warnings are *fixed*. So 'Fixes' looks the most suitable here.
> 
> > Also, After this patch there are tons of misc-related warnings
> > by the luacheсk without config. So, I think that this patch should be
> > squashed with the next one.
> 
> Yes, these changes are not atomic. I can simply reorder the patches, if
> you insist, but I definitely don't want to squash them into a single
> one. The reason is that one is about "enabling" the regular check for
> LuaJIT-related sources (#5470), but as a result of the applying
> metrics-related series, the introduced chunks have broken luacheck
> again. The new warnings are reported in a separate issue (#5631).

OK, LGTM.

> 
> > Feel free to ignore.
> > 
> > > Part of tarantool/tarantool#4862
> > > Part of tarantool/tarantool#5470
> > > Follows up tarantool/tarantool#5187
> > > 
> > > Reported-by: Sergey Bronnikov <sergeyb@tarantool.org>
> > > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > > ---
> > >  .../misclib-getmetrics-capi.test.lua          | 22 ++++++---------
> > >  .../misclib-getmetrics-lapi.test.lua          | 28 +++++++++----------
> > >  2 files changed, 23 insertions(+), 27 deletions(-)
> > > 
> 
> <snipped>
> 
> > > diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> > > index 959293d..d54caac 100755
> > > --- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> > > +++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua

<snipped>

> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun