Hammerspoon: luacheck for Travis?

[latenitefilms]

I just discovered luacheck.

I wonder if something like this is worth including as part of the work that Travis does?

[cmsj]

I guess it would depend what the goal is - right now a luacheck run on our entire extensions tree finds 2123 warnings and 0 errors.

I would hope that people aren't landing lua code that contains errors, since that would cause their code to not run at all, but are we suggesting here that we should drive the warnings down to zero? I'm not opposed to the goal, but I'm also not sure I have the time to fix 2123 warnings :)

[latenitefilms]

I was more thinking that it could be a good way to help detect typo's, unused variables, etc. before pull requests are committed to the master?

I've never used luacheck before, and I still have no idea about Travis - but when I get some time, I'll have a proper play with both.

[cmsj]

so for that kind of thing I think it would want to be a bot that posts comments back on the review at the relevant lines of code. That's getting a bit complex, integration-wise - someone would need to write a bot that monitors for PRs and then runs luacheck against them, and then posts comments back in the right form. I'm afraid I don't have time for that, but if someone else does, great!

[cmsj]

(I'm also not really sure what to do with this Issue - I absolutely wouldn't hate having the thing you suggest, but I'm very sceptical that anyone will have the time/energy to do the work, so I'm leaning towards closing it. Thoughts?)

[latenitefilms]

I wonder if this is something @cailyoung would be interested in helping me do?

[cailyoung]

This could be achievable with a code coverage tool rather than a CI like Travis if you want warnings or errors flagged on PRs.

That said you could also choose to run it locally in your editor.

I can take a look at your options tonight maybe

[latenitefilms]

Legend, thanks @cailyoung !

[cailyoung]

https://stickler-ci.com/ Perhaps

[cmsj]

@cailyoung good call on Stickler. They don't currently support Lua, but I've enabled it for our bash/python bits, and asked them if Lua support is something they'd consider :)

[cailyoung]

It's based on lint-review which is open to PRs to add languages. I think the problem is that it's a Python package and luacheck is, weirdly, written in lua :)

[cmsj]

FWIW, I've filed https://github.com/markstory/lint-review/pull/164 to add support for luacheck to lint-review. I spoke to Stickler on Twitter and they suggested that there is still additional work for them to do to get it enabled in the production service, but it's a start, and hopefully it'll happen :)

[cmsj]

Furtherly FWIW, that PR is now merged, and sticker-ci.com should support Lua in a few days. Yay!

[latenitefilms]

Amazing! Awesome work!

This will be fantastic for future PR’s, but what do you want to do about existing Lua code? Is it worth copying and pasting all the existing warnings into a new issue, then people can slowly tick them off whenever they have time? Basically make a to-do list of warnings to investigate?

[cailyoung]

Technical debt is difficult to address. It’s up to you folks how you want to deal with this but given the large amount of false negatives (luacheck cannot traverse requires very well) you might be better off just drawing a line here and waiting for further PRs/merges to clean up the codebase.

Alternative would halt progress until you’ve dealt with the huge backlog.

Perhaps open a ticket/branch that will catch all historic lint warnings and chumps like me can chip away at it in their spare time?

[asmagill]

The way I've dealt with this for Objective-C code is to hold off on cleaning out the warnings for a module until making actual updates or changes to it... something more than just a few line bug fix, but if there is a significant addition, I'll go through and clean up the warnings as well.

My thought is that until we figure out what defaults we want to recommend people use (e.g. add hs as a known global, etc.) we shouldn't put too much effort into looking at existing code that works; of course if you fix a bug, then by all means go ahead and do more as well.

[latenitefilms]

Works for me! However, I still reckon there's some benefit of having all the current warnings listed in an issue somewhere for reference.

[cmsj]

Ok, and stickler-ci.com updated with luacheck support, and it works, see: https://github.com/Hammerspoon/hammerspoon/pull/1613 😁

As for cleaning up the existing stuff, I think there's too much to try and dig through, so for now let's leave it, and as PRs come in with stickler-ci reports, we can look at what we want to put in a .luacheckrc to keep things sane.