Skip to content
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

Add pre-commit hook to enable Luacheck for pre-commit. #48

Merged
merged 1 commit into from Jan 18, 2022

Conversation

mblayman
Copy link

@mblayman mblayman commented Jan 17, 2022

Hello! I've been working with the maintainer of pre-commit to add official Lua support for the pre-commit tool. With pre-commit, a developer or team can choose to run hooks that will execute before a change is committed to git.

Support for Lua in pre-commit was literally added an hour ago as of this PR submission in pre-commit/pre-commit#2158, so this change is hot off the presses.

I believe that Luacheck would be a fantastic tool to work with pre-commit since running lint checks is a very common thing to do before committing.

This PR adds a hooks definition file that will allow any project to use Luacheck with pre-commit. I ran a local test on one of my projects to confirm the behavior. I've included the failing and passing output below to show that the hook is working.

Thanks for the consideration.

Failing output:

atlas git:(main) ✗ pre-commit try-repo ../luacheck luacheck
===============================================================================
Using config:
===============================================================================
repos:
-   repo: ../luacheck
    rev: 07cee87da56787e005b35019ddaa5eadf127655b
    hooks:
    -   id: luacheck
===============================================================================
[INFO] Initializing environment for ../luacheck.
[INFO] Installing environment for ../luacheck.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Luacheck.................................................................Failed
- hook id: luacheck
- exit code: 1

Checking src/atlas/main.lua                       3 warnings

    src/atlas/main.lua:19:9: unused variable parser
    src/atlas/main.lua:20:9: variable parser is never set
    src/atlas/main.lua:20:9: variable parser was previously defined on line 19

Total: 3 warnings / 0 errors in 1 file

Passing output:

pre-commit try-repo ../luacheck luacheck
===============================================================================
Using config:
===============================================================================
repos:
-   repo: ../luacheck
    rev: 07cee87da56787e005b35019ddaa5eadf127655b
    hooks:
    -   id: luacheck
===============================================================================
[INFO] Initializing environment for ../luacheck.
[INFO] Installing environment for ../luacheck.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Luacheck.................................................................Passed

@UncombedCoconut
Copy link

Disclaimer: I'm not a Luacheck developer.

Congrats on landing the change to pre-commit. And luachck+pre-commit is a powerful combo; I work on a project that uses it.
I think the benefit of this PR might have gotten lost in the details: it means any project using pre-commit can set up their rule like

- repo: https://github.com/lunarmodules/luacheck.git
  rev: 07cee87da56787e005b35019ddaa5eadf127655b # or just 0.26.0, once it's tagged
  hooks:
    - id: luacheck

instead of this

- repo: local
-   id: luacheck
    name: Luacheck
    description: Lint and static analysis of Lua code
    entry: luacheck
    language: lua
    types: [lua]

-- and the former would also take care setting up Luacheck for contributors who don't have it installed.
I'd probably adopt that in my project.

One question, though: is there any harm in saying language: system so that an unreleased version of pre-commit is not required? Luacheck has a shebang (#!) line in its main script. This is sufficient on Linux and I assume macOS. Does Windows ruin everything?

@mblayman
Copy link
Author

mblayman commented Jan 18, 2022

Thanks, @UncombedCoconut, that's a good observation. I appreciate you highlighting how this is valuable. You're right that this version of a pre-commit does all the work to install Luacheck on the user's behalf in an isolated luarocks tree.

I failed to describe the other side of this equation where a project wants to use this. If this hook configuration is merged, then a project could add a .pre-commit-config.yaml with a configuration close to what you listed:

repos:
- repo: https://github.com/lunarmodules/luacheck
  rev: 07cee87da56787e005b35019ddaa5eadf127655b # or just 0.26.0, once it's tagged
  hooks:
    - id: luacheck

pre-commit likes to work off a specific tag or commit so a user could whatever the merge commit is until the next time this repo is tagged.

You're right that I'm jumping the gun a bit by using an unreleased version of pre-commit. The maintainer of that tool releases often, so this could be merged after that public release to avoid any problems in usage.

As for Windows, the maintainer mentioned that things worked once he got stuff installed, but I don't know if checked against any tools. Does Luarocks install an .exe if the script is running on Windows? I'm not sure how that works.

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I've looked it over some and it seems to make sense. It seems like the outstanding issue(s) of running LuaRocks on Windows and/or needing a pre-release of pre-commit are things that can be worked out on the pre-commit end of things and aren't really relevant to this PR. Either the commit where this is merged or (when it happens) the next tagged release should include this for those that want it.

@alerque
Copy link
Member

alerque commented Jan 18, 2022

Not 100% on topic for this repo, but as long as you are looking at the Lua ecosystem let me mention busted is probably the next most relevant thing to add for Lua projects. Not all projects will want to run regression tests before committing, but for those that do busted is the most likely tool for the job.

@alerque alerque merged commit 0d41512 into lunarmodules:master Jan 18, 2022
@alerque alerque added the enhancement New feature or request label Jan 18, 2022
@mblayman mblayman deleted the pre-commit branch January 18, 2022 14:29
@mblayman
Copy link
Author

@alerque, thank you for the speedy review and merge!

@mblayman
Copy link
Author

As a bit of follow up here, the latest pre-commit version was released already so the above comments are depending on a pre-release version are, thankfully, no longer an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

3 participants