@okraits I merged the two tasks into one becuase it would be time-consuming to enforce having two tasks in which one is dependent on the other (we may get two students claiming both tasks concurrently) so in this case it made sense to merge the two in one

👍

@nemesisdesign I had a look at this and noticed some things:

• Only the tests are checked by luacheck, not the actual module code
• Warnings about redefined variables, implicitly defined variables and unused arguments and loop variables are suppressed - but those can still lead to bugs and unused code
• No warnings and errors have been fixed in the actual module code
• thus luacheck integration in editors still reports many warnings and errors as those warnings mentioned above are not suppressed normally

I'm not sure if this is wanted. But maybe I'm wrong.

@okraits no I think it was just not under my radar, I'm totally in favour of making the check stricter and keep the code cleaner

So let's reopen this issue so that things can get improved.

@okraits is this still relevant?

@nemesisdesign Yes, of course.

Was planning on working on this, just had a few queries:

• Only the tests are checked by luacheck, not the actual module code

Is this something that needs to be fixed (I couldn't tell if this was wanted or not)
Also, are there any lints that you still wish to skip (currently -d -a -r flags are set)?

• Only the tests are checked by luacheck, not the actual module code
  Is this something that needs to be fixed (I couldn't tell if this was wanted or not)

This is not wanted and needs to be fixed.

Also, are there any lints that you still wish to skip (currently -d -a -r flags are set)?

I would remove all three flags if possible.

For convenience, you can keep the -a flag for now.

@okraits
I've come up with the following notes whilst working on this issue:

• Old Modules
  The modules "uci" and "lfs" etc. are based on the old Lua 5.0/5.1 way of creating modules (modules("*", package.seeall)) - Is this something that should be updated?
  This also triggers luacheck errors as these modules rely on importing the functions into a global namespace

• Tests require globals definitions
  The unit tests require the testing table be a global variable, however this triggers a fail with Luacheck (See below for resolution)

Idea:
I was thinking, how about introducing a .luacheckrc file in regards to fixing a few of these problems, and also allowing us to remove the -a flag?

The file could be set out as such:

ignore = {"213/_.*"} -- unused Loop Variables beginning with underscore will be ignored, keeping readability
files["openwisp-config/tests"] = {
    ignore = {"11./Test.*"} -- globals relating to defining unittests are ignored
}

This will ensure a strict code linting, whilst also working around the issues we currently have.

* Old Modules
  The modules "uci" and "lfs" etc. are based on the old Lua 5.0/5.1 way of creating modules (`modules("*", package.seeall)`) - Is this something that should be updated?
  This also triggers luacheck errors as these modules rely on importing the functions into a global namespace

This means that those reported luacheck errors are in the external modules we use, not in our code, right? That would mean that those errors have to be fixed upstream. This has to be done, but it is out of scope of this PR:

* Tests require globals definitions
  The unit tests require the testing table be a global variable, however this triggers a fail with Luacheck (See below for resolution)

Idea:
I was thinking, how about introducing a .luacheckrc file in regards to fixing a few of these problems, and also allowing us to remove the -a flag?

The file could be set out as such:

ignore = {"213/_.*"} -- unused Loop Variables beginning with underscore will be ignored, keeping readability
files["openwisp-config/tests"] = {
    ignore = {"11./Test.*"} -- globals relating to defining unittests are ignored
}

This will ensure a strict code linting, whilst also working around the issues we currently have.

A very good idea! Go ahead! 😀