[Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib*
Sergey Kaplun
skaplun at tarantool.org
Tue Feb 16 19:36:05 MSK 2021
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 at 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 at tarantool.org>
> > > Signed-off-by: Igor Munkin <imun at 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
More information about the Tarantool-patches
mailing list