-
Notifications
You must be signed in to change notification settings - Fork 44
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
Use pattern matching to serialize bool values #104
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,10 +36,6 @@ defmodule Thrift.Protocols.Binary do | |
def int_type({:set, _}), do: 14 | ||
def int_type({:list, _}), do: 15 | ||
|
||
defp bool_to_int(false), do: 0 | ||
defp bool_to_int(nil), do: 0 | ||
defp bool_to_int(_), do: 1 | ||
|
||
defp to_message_type(:call), do: 1 | ||
defp to_message_type(:reply), do: 2 | ||
defp to_message_type(:exception), do: 3 | ||
|
@@ -53,10 +49,9 @@ defmodule Thrift.Protocols.Binary do | |
def serialize(_, nil) do | ||
[] | ||
end | ||
def serialize(:bool, value) do | ||
value = bool_to_int(value) | ||
<<value::8-signed>> | ||
end | ||
def serialize(:bool, nil), do: <<0::8-signed>> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I think about it - should this clause ever get called? If a value is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a boolean field in a struct is nil, that field would be omitted. That logic isn't represented in this generic serialization because it doesn't handle structs. It passes them off to presumably-generated struct serializers. (See If you serialize a list of bools, we would treat a nil in that list as falsey. serialize({:list, :bool}, [nil, false, true]) |> :erlang.iolist_to_binary
# <<2, 0, 0, 0, 3, 0, 0, 1>>
# | \--------/ | | \- true
# | | | \---- false
# | | \------- nil
# | \--------------- length = 3
# \---------------------- element type = bool Note that this doesn't currently work because of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think implicit here is whether we want to watch to support "truthy" / "falsey" inputs or if we should explicitly require There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pguillory's comment makes sense to me. @jparise raises a good point, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah good point. If we're not sure, it might be better to start strict, true/false only. We can become less strict later without breaking applications. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point @pguillory. Related - some thrift clients (I know the Ruby client does this) provide validation functionality - e.g., are required fields set, is a string field actually a string, etc. It's slow but it's usually optional. I typically enable it for testing but disable it in production. This might be something to add a TODO for later? |
||
def serialize(:bool, false), do: <<0::8-signed>> | ||
def serialize(:bool, _), do: <<1::8-signed>> | ||
def serialize(:i8, value) do | ||
<<value::8-signed>> | ||
end | ||
|
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.
This clause should not exist. The way we serialize nil depends on its context.
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.
That sounds right to me. I'll remove it in its own PR so we can track the specific motivation.
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.
This clause is removed in #105.