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

Support Lua 5.2 #21

Merged
merged 5 commits into from Apr 25, 2016

Conversation

Projects
None yet
2 participants
@jmaslak
Contributor

jmaslak commented Apr 16, 2016

Howdy, I got your module as part of the CPAN Pull Challenge.

In trying to install it on my machine, I found that it's fairly particular about which version of Lua is installed - it needs 5.1 while I had 5.2. This patch adds support for 5.2, which required slight modifications to the code (the Lua API changed a bit) and to the test scripts (some scripts that work in 5.1 don't work in 5.2 without slight modifications).

I validated that the tests do pass on 5.1 as well. Please let me know if you would like me to approach any of this a different way - I'm glad to refactor to fit your project appropriately.

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Apr 18, 2016

Owner

Hi there! Thanks for sending this my way; I'll look over the changes and leave my comments on there.

Owner

hoelzro commented Apr 18, 2016

Hi there! Thanks for sending this my way; I'll look over the changes and leave my comments on there.

Show outdated Hide outdated Lua.xs Outdated
Show outdated Hide outdated Lua.xs Outdated
Show outdated Hide outdated Lua.xs Outdated
@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Apr 18, 2016

Owner

I've left a few comments; overall, it looks good! I just suggested some small stylistic changes and some cleanup that I myself neglected to do. =)

Owner

hoelzro commented Apr 18, 2016

I've left a few comments; overall, it looks good! I just suggested some small stylistic changes and some cleanup that I myself neglected to do. =)

@jmaslak

This comment has been minimized.

Show comment
Hide comment
@jmaslak

jmaslak Apr 18, 2016

Contributor

Great comments - I agree with all your comments and will put together another commit this week. Good catch on the stdio.h - I had it in there during some debugging and forgot to rip it back out.

Contributor

jmaslak commented Apr 18, 2016

Great comments - I agree with all your comments and will put together another commit this week. Good catch on the stdio.h - I had it in there during some debugging and forgot to rip it back out.

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Apr 25, 2016

Owner

@jmaslak Looks good! Thanks for working on this!

Owner

hoelzro commented Apr 25, 2016

@jmaslak Looks good! Thanks for working on this!

@hoelzro hoelzro merged commit 3b026bd into hoelzro:master Apr 25, 2016

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