-
Notifications
You must be signed in to change notification settings - Fork 8
feat: bump luerl to 1.5.0 (#86) #88
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
Conversation
|
Thank you for the PR! Running the tests now |
|
I think there might have been some problem with builds.hex.pm right when running the builds. Should they be retried? |
|
There seem to be a dialyzer check that's not passing. It's related to defp illegal_index([:_G | keys]), do: illegal_index(keys)
defp illegal_index(["_G" | keys]), do: illegal_index(keys)
defp illegal_index(keys) do
{:illegal_index, nil, Enum.join(keys, ".")}
endIt seems that Do you think it is safe to just remove the line 288? |
|
Looking at it once more, it seems that just removing 288 won't be enough, since defp illegal_index(["_G" | keys]), do: illegal_index(keys)
defp illegal_index(keys) do
{:illegal_index, nil,
keys
|> Enum.map(fn key ->
if(is_tuple(key)) do
inspect(key)
else
key
end
end)
|> Enum.join(".")}
end |
|
@davydog187 what are the chances of this getting merged? Mostly looking at the UTF-8 changes this PR addresses :) |
|
I'll get this pushed through this week. Apologies for the delay, we're on the other side of a big launch and just getting back to OSS |
| {:illegal_index, nil, | ||
| keys | ||
| |> Enum.map(fn key -> | ||
| if(is_tuple(key)) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if(is_tuple(key)) do | |
| if is_tuple(key) do |
| Failed to compile Lua! | ||
| Failed to tokenize: illegal token on line 1: \"hi) | ||
| Line 1: syntax error near '<\\34>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely not right
|
hello @bu6n sorry for the delay on reviewing this. It still seems like something is off, given the changes to the tests. I can take a look if you'd like |
|
Ok so there seems to be a regression in Luerl itself, I'll send a patch to Luerl |
|
For reference: {1, :luerl_scan, {:user, ~c"syntax error near '<\\34>'"}} |
|
PR sent upstream to Luerl rvirding/luerl#221 |
|
thank you! |
This bumps the luerl dependency to 1.5.0 and makes tests pass again.
Failing tests were either due to:
The changelog mentions other things as well, but I'm not sure if these influence this library or not.
closes #86