Conversation
tests/repository_test.py
Outdated
| 'entry': 'lua', | ||
| 'language': 'lua', | ||
| 'args': ['-e', 'require "inspect"; print("hello world")'], | ||
| 'additional_dependencies': ['inspect'], |
There was a problem hiding this comment.
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.
|
@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 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! |
asottile
left a comment
There was a problem hiding this comment.
overall seems fine, I haven't looked at the comments in the actual PR yet so I will address those next!
|
really excited about this! seems cool!
yep -- that'll happen in https://github.com/pre-commit/pre-commit.com
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 |
|
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!) |
|
a thought I just had -- perhaps |
pre_commit/languages/lua.py
Outdated
| # so ensure the directory exists. | ||
| os.makedirs(envdir, exist_ok=True) | ||
|
|
||
| make_cmd = ['luarocks', '--tree', envdir, 'make'] |
There was a problem hiding this comment.
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.
|
@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 Also, it looks like the return for I could potentially provide an implementation for |
|
so for lua it might look like this:
|
|
I made Assuming the tests pass, I think this is ready for another review. I hope this is getting close to the finish line. 😁 |
|
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 I'll work on implementing this! thank you for the basis, this is really helpful! |
|
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. |
|
alrighty this should be good to go -- I tried it out with |
|
I also checked and it works on windows! (if you can manage to get lua installed which was a big struggle) |
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?