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 Lua language support #2158

Merged
merged 2 commits into from Jan 17, 2022
Merged

Add Lua language support #2158

merged 2 commits into from Jan 17, 2022

Conversation

mblayman
Copy link
Contributor

This PR adds support for Lua as a language using Luarocks for package management.

I have the Lua tests working locally. I suspect that CI is going to break until I change something about the azure-pipelines.yml.

Also, this PR has zero documentation about the new language support. Is documentation done in a different repo? If so, how can I help get something appropriate written?

'entry': 'lua',
'language': 'lua',
'args': ['-e', 'require "inspect"; print("hello world")'],
'additional_dependencies': ['inspect'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inspect pretty prints tables (because the default print of a table spits out a fairly useless memory address). This seemed like a tame additional dependency to check against.

testing/get-lua.sh Outdated Show resolved Hide resolved
testing/get-lua.sh Outdated Show resolved Hide resolved
pre_commit/languages/lua.py Outdated Show resolved Hide resolved
@mblayman
Copy link
Contributor Author

@asottile, things are now working with 100% coverage on POSIX CI.

What strategy would you like me to apply for Windows? I think it's likely to be quite rough to get tests running for Windows (e.g., the Lua docs automatically assume the presence of make when building from source so I'm not even sure where to start for this Windows setup. cygwin?). I see some tests that are for POSIX only. Should I do that for the tests? If I did that, I think the coverage on the Windows run would be less than 100% so I'm not sure how to handle that.

I've invested a fair amount of time into this PR, and I'd like to see it merged ultimately because I think it will be valuable for Lua users, so please let me know what I can do to help get this branch where it needs to be.

Thanks in advance for any guidance on this!

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

overall seems fine, I haven't looked at the comments in the actual PR yet so I will address those next!

pre_commit/languages/lua.py Outdated Show resolved Hide resolved
pre_commit/languages/lua.py Outdated Show resolved Hide resolved
pre_commit/languages/lua.py Outdated Show resolved Hide resolved
pre_commit/languages/lua.py Show resolved Hide resolved
pre_commit/languages/lua.py Outdated Show resolved Hide resolved
pre_commit/languages/lua.py Outdated Show resolved Hide resolved
testing/get-lua.sh Outdated Show resolved Hide resolved
testing/get-lua.sh Outdated Show resolved Hide resolved
testing/get-lua.sh Outdated Show resolved Hide resolved
tests/languages/lua_test.py Outdated Show resolved Hide resolved
@asottile
Copy link
Member

really excited about this! seems cool!

Also, this PR has zero documentation about the new language support. Is documentation done in a different repo? If so, how can I help get something appropriate written?

yep -- that'll happen in https://github.com/pre-commit/pre-commit.com

What strategy would you like me to apply for Windows?

hmm yeah it would be ideal to test it on windows as well -- but that can come later if you'd like. the tests really only need to work in azure pipelines so I would see if someone else has a lua setup that works well for them

@mblayman
Copy link
Contributor Author

Thanks for the thorough review. I'll try to address the comments in the next couple of days.

@asottile
Copy link
Member

Thanks for the thorough review. I'll try to address the comments in the next couple of days.

no rush! and thanks for working on this! (I reviewed it on stream today if you want to watch the vod, and gave you a nice shoutout!)

@asottile
Copy link
Member

a thought I just had -- perhaps language_version does make sense for lua if there's potentially many different point releases installed (similar to python?)

# so ensure the directory exists.
os.makedirs(envdir, exist_ok=True)

make_cmd = ['luarocks', '--tree', envdir, 'make']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched from build to make because I learned that build does use the source.url defined in the rockspec. make is like build, but it only uses the local directory content.

@mblayman
Copy link
Contributor Author

@asottile, I finally got a passing build by skipping the Windows side for now. I believe I've addressed almost all of your comments now.

Can you help me understand the purpose of the language_version stuff? I'm not sure what that's supposed to enable in pre-commit.

Also, it looks like the return for get_default_version is either the value of C.DEFAULT or the name of the programming language's executable that is discovered. Did I get that right?

I could potentially provide an implementation for get_default_version, but I don't understand why that would help right now.

@asottile
Copy link
Member

so language_version is intended to allow hooks to specify that they need a specific version of a programming language

for lua it might look like this:

  • default_language_version() -> discover all of the lua / lua#.# executables and pick the newest or most appropriate one
  • allow the user to set language_version which would then use a specific lua#.# executable to do the installation and execution

pre_commit/languages/lua.py Outdated Show resolved Hide resolved
pre_commit/languages/lua.py Outdated Show resolved Hide resolved
pre_commit/languages/lua.py Outdated Show resolved Hide resolved
pre_commit/store.py Outdated Show resolved Hide resolved
@mblayman
Copy link
Contributor Author

I made language_version work with the _find_lua function so I think this will now work if users request a different language version.

Assuming the tests pass, I think this is ready for another review. I hope this is getting close to the finish line. 😁

@asottile
Copy link
Member

alright, apologies for taking so long on this -- I'm going to finish it up for you since I've taken so much of your time here -- thank you!

I did a little research last night and it looks like even though multiple lua versions could be installed, luarocks (or at least the current version of LTS releases) doesn't have a good way of selecting between the versions. As such, I think we'll leave out language_version support for now and just support whatever version luarocks indicates it is installing for.

I'll work on implementing this! thank you for the basis, this is really helpful!

@mblayman
Copy link
Contributor Author

Great! I appreciate the help on this.

My endgame here is to make two hooks: one for LuaCheck and one for LuaFormatter.

If I can help test this out with a real hook, I'd be happy to be a tester/initial consumer. Since this is a pre-released version of pre-commit on a branch, I would appreciate any config pointers if you have them.

@asottile
Copy link
Member

alrighty this should be good to go -- I tried it out with luacheck (in the test) mostly so I could remove the lua binary finding bits -- now the whole thing works just using luarocks

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit b22b313 into pre-commit:master Jan 17, 2022
@asottile
Copy link
Member

I also checked and it works on windows! (if you can manage to get lua installed which was a big struggle)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants