[Tarantool-patches] [PATCH v5 06/10] Fix luacheck warnings in src/box/lua/
Sergey Bronnikov
sergeyb at tarantool.org
Thu May 28 18:16:06 MSK 2020
Igor,
thanks for review, see my comments inline.
All changes are in branch.
On 19:54 Wed 27 May , Igor Munkin wrote:
> Sergey,
>
> Thanks for the patch! There is a mess with suppressions (branch is OK),
> so I just left only relevant part.
>
> On 12.05.20, sergeyb at tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb at tarantool.org>
> >
> > Closes #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > ---
> > .luacheckrc | 42 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 42 insertions(+)
> >
> > diff --git a/.luacheckrc b/.luacheckrc
> > index 3cd05a254..e5c4c4509 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -29,6 +29,48 @@ files["extra/dist/tarantoolctl.in"] = {
> > globals = {"box", "_TARANTOOL"},
> > ignore = {"212/self", "122", "431"}
> > }
> > +files["**/*.lua"] = {
>
> Again, spaces vs tabs.
Fixed.
> > + globals = {"box", "_TARANTOOL", "help", "tutorial"},
>
> OK, box and _TARANTOOL globals are widely used but AFAICS help and
> tutorial occurs only in src/lua/help.lua and src/box/lua/console.lua, so
> there is no need to suppress it source-wide.
Fixed.
> > + ignore = {"212/self", "122", "143", "142"}
>
> I see no warning with [122] and [142] code, so they seem to be excess
> here. Furthermore why do we need to suppress [143] globally if we don't
> use vanilla luacheck?
It is not so, we use vanilla luacheck, look at the changes in
.travis.mk. Anyway removed supressions for W122 and W142.
> > +}
> > files["src/lua/*.lua"] = {ignore = {"212/self"}}
>
> If unused self argument is suppressed source-wide, the suppression above
> can be dropped.
Agree, removed.
> > files["src/lua/init.lua"] = {globals = {"dostring"}}
> > files["src/lua/swim.lua"] = {ignore = {"431"}}
> > +files["src/box/lua/console.lua"] = {ignore = {"212"}}
>
> Well, why do we have so much unused args here? Can we fix such warnings?
> Feel free to file a follow-up issue for this.
created issue [2]
> > +files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
>
> The suppression above is excess since the issue is fixed via inline one.
Removed.
> > +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "411"}}
>
> One general comment: please sort suppressed codes, it's just more
> convenient for further maintenance.
Done in a whole .luacheckrc.
> [411] warning can be fixed in a very simple way: just remove local prior
> to create_transport on line 919 (consider the diff at the end). As a
> result [411] suppression can be removed.
Proposed fix brokes tests, so I just supressed warning inline.
> > +files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "432"}}
>
> Why do we need to explicitly set tonumber64 as global and does not set
> bit? Please choose one: either mention all globals used in sources to
> prepare the sources to be checked against vanilla luacheck or just omit
> such occurences if we're going to use our fork in CI.
Without 'tonumber64' in globals warning W113 triggered. See [1].
> As a result, after applying the following diff everything is OK:
>
> ================================================================================
>
> diff --git a/.luacheckrc b/.luacheckrc
> index a141e76ef..716f376b8 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -30,13 +30,11 @@ files["extra/dist/tarantoolctl.in"] = {
> ignore = {"212/self", "122", "431"}
> }
> files["**/*.lua"] = {
> - globals = {"box", "_TARANTOOL", "help", "tutorial"},
> - ignore = {"212/self", "122", "143", "142"}
> + globals = {"box", "_TARANTOOL"},
> + ignore = {"212/self"},
> }
> -files["src/lua/*.lua"] = {ignore = {"212/self"}}
> files["src/lua/init.lua"] = {globals = {"dostring"}}
> files["src/lua/swim.lua"] = {ignore = {"431"}}
> -files["src/box/lua/console.lua"] = {ignore = {"212"}}
> -files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
> -files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "411"}}
> -files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "432"}}
> +files["src/box/lua/console.lua"] = {globals = {"help"}, ignore = {"212"}}
> +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432"}}
> +files["src/box/lua/schema.lua"] = {ignore = {"431", "432"}}
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 3f2a9df81..646529173 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -916,7 +916,7 @@ end
> -- Now it is necessary to have a strong ref to callback somewhere or
> -- it is GC-ed prematurely. We wrap stop() method, stashing the
> -- ref in an upvalue (stop() performance doesn't matter much.)
> -local create_transport = function(host, port, user, password, callback,
> +create_transport = function(host, port, user, password, callback,
> connection, greeting)
> local weak_refs = setmetatable({callback = callback}, {__mode = 'v'})
> local function weak_callback(...)
>
> ================================================================================
>
> I checked the src/box/lua/*.lua sources with our luacheck fork:
> | $ tarantoolctl rocks install luacheck
> | $ $ .rocks/bin/luacheck --codes --config .luacheckrc src/box/lua/*.lua
> | Checking src/box/lua/console.lua OK
> | Checking src/box/lua/feedback_daemon.lua OK
> | Checking src/box/lua/key_def.lua OK
> | Checking src/box/lua/load_cfg.lua OK
> | Checking src/box/lua/merger.lua OK
> | Checking src/box/lua/net_box.lua OK
> | Checking src/box/lua/schema.lua OK
> | Checking src/box/lua/session.lua OK
> | Checking src/box/lua/tuple.lua OK
> | Checking src/box/lua/upgrade.lua OK
> | Checking src/box/lua/xlog.lua OK
> |
> | Total: 0 warnings / 0 errors in 11 files
>
> <snipped>
>
> > --
> > 2.23.0
> >
>
> --
> Best regards,
> IM
[1] https://gitlab.com/tarantool/tarantool/-/jobs/571452395
[2] https://github.com/tarantool/tarantool/issues/5032
More information about the Tarantool-patches
mailing list