[Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/tarantoolctl.in
Igor Munkin
imun at tarantool.org
Wed Apr 15 18:35:26 MSK 2020
Sergey,
Thanks for the patch! I started with this one, since it's simple and I
can point the general issues related to the fix.
I see the following approach to fix the errors and warning found by
luacheck in *Lua sources*:
* If the fix is obvious and simple -- please do it.
* If the warning/erro fix leads to code complication or reduce
readability (e.g. empty if branch) -- let's add an inline ignore
comment, since it unlikely to be fixed and just burdens .luacheckrc.
* If none of the above -- add the corresponding rule to .luacheckrc.
I left inline comments for your patch and added the fix I made for
tarantoolctl at the end. Please consider them.
On 14.04.20, Sergey Bronnikov wrote:
> Closes #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> ---
>
> extra/dist/tarantoolctl.in | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> index f5a912ccd..d7f098735 100755
> --- a/extra/dist/tarantoolctl.in
> +++ b/extra/dist/tarantoolctl.in
<snipped>
> @@ -64,7 +63,7 @@ end
>
> local function check_user_level()
> local uid = os.getenv('UID')
> - local udir = nil
> + local udir
Minor: I looked at the code nearby and it looks like this initialization
can be moved below, since udir is unused prior it (consider the changes
below).
> if uid == 0 or os.getenv("NOTIFY_SOCKET") then
> return nil
> end
<snipped>
> --
> 2.23.0
>
>
> --
> sergeyb@
I tried to fix the issues by myself and dumped my step-by-step activity
below. At first let's see what luacheck reports for tarantoolctl source:
| $ luacheck --codes extra/dist/tarantoolctl.in | tail -1
| Total: 35 warnings / 0 errors in 1 file
| $ luacheck --codes extra/dist/tarantoolctl.in | \
| grep -vP '^(Total:|Checking|$)' | cut -f 6 -d ' ' | sort | uniq -c
| 1 (W112)
| 5 (W113)
| 1 (W122)
| 2 (W211)
| 3 (W212)
| 4 (W213)
| 3 (W311)
| 2 (W411)
| 13 (W431)
| 1 (W542)
According to the list of warnings[1] W113 is reported when accessing an
undefined global variable is detected. At the same time W112 is reported
when mutating an undefined global variable is detected. Considering
output these are well known Tarantool globals: box and _TARANTOOL. Let's
suppress them (now with CLI option and globally in .luacheckrc further):
| $ luacheck --codes extra/dist/tarantoolctl.in --globals box --globals _TARANTOOL | \
| grep -vP '^(Total:|Checking|$)' | cut -f 6 -d ' ' | sort | uniq -c
| 1 (W122)
| 2 (W211)
| 3 (W212)
| 4 (W213)
| 3 (W311)
| 2 (W411)
| 13 (W431)
| 1 (W542)
Well, 6 false-positive warnings are suppressed, 29 others left. Let's
see the remaining ones:
| $ luacheck --codes extra/dist/tarantoolctl.in --globals box --globals _TARANTOOL
| Checking extra/dist/tarantoolctl.in 29 warnings
|
| extra/dist/tarantoolctl.in:49:7: (W211) unused variable 'lua_arguments'
| extra/dist/tarantoolctl.in:67:11: (W311) value assigned to variable 'udir' is unused
| extra/dist/tarantoolctl.in:127:34: (W431) shadowing upvalue 'default_file' on line 38
| extra/dist/tarantoolctl.in:136:22: (W411) variable 'msg' was previously defined on line 130
| extra/dist/tarantoolctl.in:217:5: (W122) setting read-only field '?' of global 'arg'
| extra/dist/tarantoolctl.in:423:24: (W212) unused argument 'self'
| extra/dist/tarantoolctl.in:440:15: (W431) shadowing upvalue 'console_sock' on line 42
| extra/dist/tarantoolctl.in:573:11: (W431) shadowing upvalue 'console_sock' on line 42
| extra/dist/tarantoolctl.in:634:11: (W431) shadowing upvalue 'console_sock' on line 42
| extra/dist/tarantoolctl.in:695:31: (W431) shadowing upvalue 'uri' on line 9
| extra/dist/tarantoolctl.in:759:11: (W431) shadowing upvalue 'console_sock' on line 42
| extra/dist/tarantoolctl.in:793:11: (W311) value assigned to variable 'code' is unused
| extra/dist/tarantoolctl.in:819:11: (W431) shadowing upvalue 'status' on line 750
| extra/dist/tarantoolctl.in:851:11: (W211) unused variable 'spaces'
| extra/dist/tarantoolctl.in:852:21: (W411) variable 'spaces' was previously defined on line 851
| extra/dist/tarantoolctl.in:865:59: (W542) empty if branch
| extra/dist/tarantoolctl.in:878:9: (W213) unused loop variable 'id'
| extra/dist/tarantoolctl.in:895:11: (W431) shadowing upvalue 'uri' on line 9
| extra/dist/tarantoolctl.in:904:9: (W213) unused loop variable 'id'
| extra/dist/tarantoolctl.in:926:9: (W213) unused loop variable 'i'
| extra/dist/tarantoolctl.in:940:11: (W311) value assigned to variable 'key' is unused
| extra/dist/tarantoolctl.in:957:30: (W212) unused argument 'command'
| extra/dist/tarantoolctl.in:957:39: (W212) unused argument 'program'
| extra/dist/tarantoolctl.in:1324:9: (W213) unused loop variable 'no'
| extra/dist/tarantoolctl.in:1333:31: (W431) shadowing upvalue 'commands' on line 1019
| extra/dist/tarantoolctl.in:1335:18: (W431) shadowing upvalue 'self_name' on line 30
| extra/dist/tarantoolctl.in:1387:15: (W431) shadowing upvalue 'command_name' on line 32
| extra/dist/tarantoolctl.in:1388:15: (W431) shadowing upvalue 'positional_arguments' on line 47
| extra/dist/tarantoolctl.in:1388:37: (W431) shadowing upvalue 'keyword_arguments' on line 48
|
| Total: 29 warnings / 0 errors in 1 file
Considering your patch you've already fixed all W211[unused local
variable], W212[unused argument] (considering the fix proposed by Oleg)
and W213[unused loop variable] warnings. The fixes are obvious and
good. Furthermore, you've successfully fixed all W311[value assigned to
a local variable is unused] warnings (but I fixed one of them in a
slightly different way below).
If I didn't miss anything, only the following warnings are left:
| $ luacheck --codes extra/dist/tarantoolctl.in --globals box --globals _TARANTOOL | \
| grep -vP '^(Total:|Checking|$)' | cut -f 6 -d ' ' | sort | uniq -c
| 1 (W122)
| 1 (W411)
| 13 (W431)
| 1 (W542)
* extra/dist/tarantoolctl.in:136:22: (W411) variable 'msg' was previously defined on line 130
Both msg values are well-localized, so I guess we can simply rename
both occurences (consider the changes below).
* extra/dist/tarantoolctl.in:865:59: (W542) empty if branch
This warning is also false-positive, since the way the branches are
organized makes code clearer. Thereby this warning looks unlikely to
be fixed, so I added an inline suppression.
The remaining errors (read-only arg table modification and upvalue
shadowing) can be placed to .luacheckrc to be fixed in future, since
fixes seem to be non-trivial and some of them introduce changes to
function contracts/behaviour.
Omitting few lines to be added to .luacheckrc, here are my changes:
================================================================================
diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index f5a912ccd..a80ecc057 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -46,7 +46,6 @@ local instance_dir
local default_cfg
local positional_arguments
local keyword_arguments
-local lua_arguments = arg
local language
-- function for printing usage reference
@@ -64,13 +63,12 @@ end
local function check_user_level()
local uid = os.getenv('UID')
- local udir = nil
if uid == 0 or os.getenv("NOTIFY_SOCKET") then
return nil
end
-- local dir configuration
local pwd = os.getenv('PWD')
- udir = pwd and pwd .. '/.tarantoolctl'
+ local udir = pwd and pwd .. '/.tarantoolctl' or nil
udir = udir and fio.stat(udir) and udir or nil
-- or home dir configuration
local homedir = os.getenv('HOME')
@@ -127,15 +125,15 @@ end
local function load_default_file(default_file)
if default_file then
local env = setmetatable({}, { __index = _G })
- local ufunc, msg = loadfile(default_file)
+ local ufunc, loaderr = loadfile(default_file)
-- if load fails - show the last 10 lines of the log file
if not ufunc then
- log.error("Failed to load defaults file: %s", msg)
+ log.error("Failed to load defaults file: %s", loaderr)
end
debug.setfenv(ufunc, env)
- local state, msg = pcall(ufunc)
+ local state, err = pcall(ufunc)
if not state then
- log.error('Failed to execute defaults file: %s', msg)
+ log.error('Failed to execute defaults file: %s', err)
end
default_cfg = env.default_cfg
instance_dir = env.instance_dir
@@ -420,7 +418,7 @@ local cat_formats = setmetatable({
json = cat_json_cb,
lua = cat_lua_cb,
}, {
- __index = function(self, cmd)
+ __index = function(_, cmd)
error(("Unknown formatter '%s'"):format(cmd))
end
})
@@ -790,7 +788,7 @@ end
local function eval()
local console_sock_path = uri.parse(console_sock).service
local filename = arg[3]
- local code = nil
+ local code
if filename == nil then
if stdin_isatty() then
log.error("Usage:")
@@ -848,7 +846,6 @@ end
-- xlog / snap file, so even when it stops on LSN >= @a opts.to on
-- a current file a next file will be processed.
local function filter_xlog(gen, param, state, opts, cb)
- local spaces = opts.spaces
local from, to, spaces = opts.from, opts.to, opts.space
local show_system, replicas = opts['show-system'], opts.replica
@@ -862,7 +859,7 @@ local function filter_xlog(gen, param, state, opts, cb)
elseif (lsn < from) or (lsn >= to) or
(not spaces and sid and sid < 512 and not show_system) or
(spaces and (sid == nil or not find_in_list(sid, spaces))) or
- (replicas and not find_in_list(rid, replicas)) then
+ (replicas and not find_in_list(rid, replicas)) then -- luacheck: ignore
-- pass this tuple
else
cb(record)
@@ -875,7 +872,7 @@ local function cat()
local cat_format = opts.format
local format_cb = cat_formats[cat_format]
local is_printed = false
- for id, file in ipairs(positional_arguments) do
+ for _, file in ipairs(positional_arguments) do
log.error("Processing file '%s'", file)
local gen, param, state = xlog.pairs(file)
filter_xlog(gen, param, state, opts, function(record)
@@ -901,7 +898,7 @@ local function play()
if not remote:wait_connected() then
error(("Error while connecting to host '%s'"):format(uri))
end
- for id, file in ipairs(positional_arguments) do
+ for _, file in ipairs(positional_arguments) do
log.info(("Processing file '%s'"):format(file))
local gen, param, state = xlog.pairs(file)
filter_xlog(gen, param, state, opts, function(record)
@@ -923,7 +920,7 @@ local function play()
end
local function find_arg(name_arg)
- for i, key in ipairs(arg) do
+ for _, key in ipairs(arg) do
if key:find(name_arg) ~= nil then
return key
end
@@ -937,7 +934,7 @@ local function rocks()
local util = require("luarocks.util")
local command_line = require("luarocks.cmd")
local options = keyword_arguments
- local key = nil
+ local key
if options["only-server"] ~= nil then
key = find_arg("only%-server")
else
@@ -954,7 +951,7 @@ local function rocks()
end
-- Tweak help messages
- util.see_help = function(command, program)
+ util.see_help = function()
-- TODO: print extended help message here
return "See Tarantool documentation for help."
end
@@ -1321,7 +1318,7 @@ local function usage_command(name, cmd)
if type(header) == 'string' then
header = { header }
end
- for no, line in ipairs(header) do
+ for _, line in ipairs(header) do
log.error(" " .. line, name)
end
end
================================================================================
[1]: https://luacheck.readthedocs.io/en/stable/warnings.html
--
Best regards,
IM
More information about the Tarantool-patches
mailing list