Use pattern matching to serialize bool values

[jparise]

This eliminates the private bool_to_int/1 function and lets us return
constant binary result values directly.

We also strictly match on just the true and false atoms.

There is a nominal performance improvement, but nothing significant.

[jparise]

The other way we could do this is to pattern match nil, false, and _ directly in the serialize(:bool, _) function. Any preference? I suppose that might be even cleaner.

[dantswain]

👍 I like the pattern matching in the function clause.

[dantswain]

I suck at using the review tool. 👍 to just using function clause pattern matching.

[jparise]

@dantswain yeah, I realize that would be the better approach as I was writing that comment. 😉

[dantswain]

Now that I think about it - should this clause ever get called? If a value is nil then it's not set and should be skipped.

[pguillory]

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 serialize(:struct, _) further down.)

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 serialize(_, nil) clause further up.

[jparise]

I think implicit here is whether we want to watch to support "truthy" / "falsey" inputs or if we should explicitly require true or false atoms (where all other inputs result in an :error).

[dantswain]

@pguillory's comment makes sense to me. @jparise raises a good point, too.

[pguillory]

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.

[dantswain]

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?

[pguillory]

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 serialize(:struct, _) further down.)

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 serialize(_, nil) clause further up.

[pguillory]

This clause should not exist. The way we serialize nil depends on its context.

[jparise]

That sounds right to me. I'll remove it in its own PR so we can track the specific motivation.

[jparise]

This clause is removed in #105.