Accept 0 and 1 as boolean values #64
Conversation
I agree, there are more issues like this, where gpb is overly generous, and there's also a general note in the README that
About
Take a look at |
|
Thanks. This PR is ready for review 😄 |
|
Looks fine. I just came to think of one more thing: The README.md has a section on Erlang-internal types and values that each protobuf language type maps to, it would be nice if |
lukebakken
force-pushed
the
feature/lrb/epb-boolean-compat
branch
from
e0b2f32
to
3946d4c
Compare
|
@tomas-abrahamsson - good catch, I'll make sure to update the I re-formatted the table to use markdown syntax. The output looks good, but the source got uglier. What do you think? |
lukebakken
force-pushed
the
feature/lrb/epb-boolean-compat
branch
from
3946d4c
to
00fdbaa
Compare
|
Nice table! The source definitely got a bit worse with super long lines, but I figure the table is read by people many more times than it is edited, so I'd say it is a change for the better anyway. |
|
OK, great. I think this PR should be all set then. Thank you for your assistance. |
| GMDecoded = gpb:decode_msg(MEncoded, ntest3, Defs), | ||
| MGDecoded = M:decode_msg(GEncoded, ntest3), | ||
| ?assertEqual(OrigMsgAtom, MMDecoded), | ||
| ?assertEqual(OrigMsgInt, GMDecoded), |
There was a problem hiding this comment.
I think this line should read: ?assertEqual(OrigMsgAtom, GMDecoded),
(Ie OrigMsgAtom instead of OrigMsgInt, making the OrigMsgInt variable unused)
I found this out during a test merge I did of all epb compat branches, it probably went unnoticed earlier due to the other bug of not catching bugs in from the separate vm stage, sorry about that :/
|
Found one issue during a test merge |
lukebakken
force-pushed
the
feature/lrb/epb-boolean-compat
branch
from
00fdbaa
to
a10c34e
Compare
|
@tomas-abrahamsson the boolean nif test is fixed, and the failure exposed a bug. I'm making one more small change, stay tuned... |
Add NIF test for 0/1 booleans, fix function_clause error in format_bool_encoder Expand section about booleans in types table. Convert to markdown table Fix boolean nif test Make gpb_true_int a one-time initialized ERL_NIF_TERM
lukebakken
force-pushed
the
feature/lrb/epb-boolean-compat
branch
from
04850f5
to
38fef38
Compare
|
OK now I think it's all set 😄 |
|
Perfect, up for merge, and will be included in the next release. |
|
This is now included in 3.24.0 |
Accept 0 and 1 for false / true boolean values.
Related to #63
Questions -
0and1asdefaultvalues, which isn't part of the PB spec. This doesn't seem to be the end of the world, though. What do you think @tomas-abrahamsson ?trueandfalseatoms? I couldn't find them, and just wanted to verify.Thanks!