New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AP_Scripting: add luacheck lua linter CI, and fix all current warnings #22887
Conversation
Result:
|
@@ -727,7 +727,7 @@ function update() | |||
end | |||
|
|||
-- send rate target | |||
local roll_degs, pitch_degs, yaw_degs, yaw_is_ef = mount:get_rate_target(MOUNT_INSTANCE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha! this is a real bug because we should be using this. I will create a fix although I'm happy for this to go in as well.
I'm sure this will annoy some developers but it is also really useful to have these checks run. I think it will lead to less bugs in our scripts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not used to lua, but from safety and CI that looks nice!
We should ignore this one too: |
added that extra ignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a comment like this:
-- LUACHECK
and only check files with that marker?
bd9b5b9
to
44a9cfe
Compare
6f19cdc
to
8d565de
Compare
After a good deal of messing about I now have this working without using the magic using actions helper. It does fail correctly. This still checks everything, however stuff that has not been fixed adds |
Adds a luacheck config and then uses that as part of CI meaning CI will fail for bad lua. EG these are all the warnings on current master Total: 1105 warnings / 0 errors in 127 files
Not a complete check yet as this currently ignores globals, some extra config is needed to tell luacheck about the extra bindings we have. #15014
I have fixed all the warnings in our current scripts. Mostly by just fixing them but in some-cases adding
-- luacheck: ignore
. I have globally turned off some white space checks, and a line length check. I have not tested any of the fixes, if there is a desire for significant testing I would prefer that we just add ignores everywhere and then fix them one at a time as we update scripts.Luacheck docs: https://luacheck.readthedocs.io/en/stable/index.html
You can run locally with
luacheck */ --config libraries/AP_Scripting/tests/luacheck.lua
to check everything. Or for exampleluacheck libraries/AP_Scripting/examples/simple_loop.lua --config libraries/AP_Scripting/tests/luacheck.lua
to check a particular file.