-
Notifications
You must be signed in to change notification settings - Fork 1
Partially revert encoders #20
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
DynamicCake
left a comment
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.
Just want some clarification on certain decisions
|
|
||
| @external(erlang, "glua_ffi", "coerce") | ||
| pub fn table(values: List(#(Value, Value))) -> Value | ||
| pub fn table(lua: Lua, values: List(#(Value, Value))) -> #(Lua, Value) { |
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.
Why does this do what luerl:encode but manually?
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.
Because of nested tables. If we try to call luerl:encode on a list that already have encoded values inside it will result in a runtime error.
src/glua_ffi.erl
Outdated
| false. | ||
|
|
||
| encode(X, St0) -> | ||
| case is_encoded(X) of |
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.
I long for the day that we don't need to check is_encoded and instead rely on the type system.
Its fine if this doesn't get fixed.
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.
I'm exploring a way of removing is_encoded, I will update this when I got it working.
6ca1b27 to
5dd4aab
Compare
|
LGTM |
No description provided.