Process initial Lua linter results & tweak its config

[Dutchman101]

Lua linter from PR #354 (merged) will mostly be used to hint PR authors about potential mistakes & check off warnings before proceeding. But we also have some interesting results from the initial scan of our entire repository.

Tasks:

• Identify the most common false positive warning types (due to MTA's Lua stack) so we can add more ignore rules, even after the last one we added: 7c135ae - the below warning log file will be helpful for this
• Triage the warning log to identify and fix issues with official resources in mtasa-resources
  Warning log: https://pastebin.mtasa.com/999998690 or see attached .txt file: LuaLinter_results.txt

To work from: Repository tree (version: 7c135ae) to keep this log relevant for a while

Any help with either points will be appreciated. Especially skimming through warnings on our existing scripts and checking if they identified real problems, is time-consuming. If you have any candidates to be added to the ignore list, please comment them.

For a list of warning ID's and types see luacheck/warnings.rst

[Disinterpreter]

In addertum to D101. All linters and static analysis software not ideal.

So we recommend listen to them, but don't trust if the situation allows.

If you see the luacheck wrong in a specific situation, please use: https://luacheck.readthedocs.io/en/stable/inline.html?highlight=ignore#inline-options

[Dutchman101]

So we recommend listen to them, but don't trust if the situation allows

yes, that's why i said: skimming through warnings on our existing scripts and checking if they identified real problems, all static analyzer output requires manual checking as most of them are false positive/not relevant

[stoneage-mta]

I've been checking for some time and so far i havent found any real problem... and IMHO we could add id 213 - (unused loop variable), and maybe all shadowing warnings (421, 422, 423, 431, 432, 433) to the ignore list.

[stoneage-mta]

It might also be worth adding 212 (unused argument) to the ignore list??? because in my opinion, in some cases like the one below, [example1] is much more newbie-friendly than [example2], and in the end it won't make any difference...

[example1]:

function bruh(weapon, ammo, ammoInClip, hitX, hitY, hitZ, hitElement)
    -- do something with hitElement

[example2]:

function bruh(_, _, _, _, _, _, hitElement)
    -- do something with hitElement

What do you think?

[Disinterpreter]

It might also be worth adding 212 (unused argument) to the ignore list??? because in my opinion, in some cases like the one below, [example1] is much more newbie-friendly than [example2], and in the end it won't make any difference...

[example1]:

function bruh(weapon, ammo, ammoInClip, hitX, hitY, hitZ, hitElement)
    -- do something with hitElement

[example2]:

function bruh(_, _, _, _, _, _, hitElement)
    -- do something with hitElement

What do you think?

For example, golang compiller deny the same things like unused variable/argument, so I think it is good. But about newbie, I think you are right.
IMHO: _ is good.

There aren't right opinion I suppose.

[sacr1ficez]

It shorten lines, which in my opinion is better than having bunch of unused stuff, causing some lines to be hella longer.

[Dutchman101]

It shorten lines, which in my opinion is better than having bunch of unused stuff, causing some lines to be hella longer.

Putting all arguments in the function is a very common practise in MTA (and already widely adopted in mtasa-resources).. not to speak about its value to beginning scripters. Because MTA differs from typical Lua in this aspect, it's related to our stack and those warnings should be ignored. I mean, i feel like conforming to this very common practise in MTA scripts takes priority over shortening lines.

[Dutchman101]

There is some good feedback over at #368 (comment), to be looked into after this stage (preferably with a new issue & project goal)

[ArranTuna]

Rule 213 (unused loop variable) needs adding because "for i, plr in ipairs(getElementsByType("player)) do" will warn on this unused loop variable on "i" which is just stupid. There are 1000 matches of this.

Rule 211 (unused local variable) with348 matches is just annoying too for example label1 is unused in this:

local label1 = guiCreateLabel ( xpos,		ypos, 50, 16,	"AC #" .. info.id,			false, tab )
local label2 = guiCreateLabel ( xpos+70,	ypos, 50, 16,	bEnabled and "ON" or "OFF", false, tab )
local label3 = guiCreateLabel ( xpos+110,	ypos, 450, 16,	"( "..info.desc.." )",		false, tab )

I don't see how anyone benefits from changing it to this when it looks less organised:

guiCreateLabel ( xpos,		ypos, 50, 16,	"AC #" .. info.id,			false, tab )
local label2 = guiCreateLabel ( xpos+70,	ypos, 50, 16,	bEnabled and "ON" or "OFF", false, tab )
local label3 = guiCreateLabel ( xpos+110,	ypos, 450, 16,	"( "..info.desc.." )",		false, tab )

[TheNormalnij]

Im used to organize large function calls like this

guiCreateLabel(
    xpos, ypos,
    50, 16,
    "AC #" .. info.id,
    false, tab
)

local smtStateLabel = guiCreateLabel(
    xpos + 70, ypos,
    50, 16,
    bEnabled and "ON" or "OFF",
    false, tab
)

local desctiptionLabel = guiCreateLabel(
    xpos + 110, ypos,
    450, 16,
    "( "..info.desc.." )",
    false, tab
)

It looks more readable and refactor-friendly. Variable names should have sense also.
Rule 213 haven't sense for me.

for i = 1, 3 do -- << looks better
    -- smt
end

for _ = 1, 3 do
    -- smt
end

[ArranTuna]

611 | A line consists of nothing but whitespace.

This rule should be disabled too because if there is a blank line inside a function and that line is indented like it should be, this rule is triggered and I can't see any logic as to why it shouldn't be properly indented.

212 | Unused argument.

Everyone above agreed to disable this 1.

@Dutchman101 So if 221, 212, 213 and 611 are added to ignore it will clear 90% of the warnings and the ones left are much more likely to be stuff worth correcting.

[TheNormalnij]

611 | A line consists of nothing but whitespace.

This rule should be disabled too because if there is a blank line inside a function and that line is indented like it should be, this rule is triggered and I can't see any logic as to why it shouldn't be properly indented.

Modern IDE's remove "garbage" spaces and tabs when you go to new line. I see no problem with it.

212 | Unused argument.

Everyone above agreed to disable this 1.

I think this rule works well only with internal resource functions and looks bad with addEventHandler. This rule helps you when you refactor something, but i prefer to see all event handler arguments. And MTA resources are all about event handling