converts :null to nil #47
Conversation
|
Hey @matreyes nice catch, thanks for your support. I think you are right, I will have to put a major version on release. I would like to ask you to write a few tests to cover the change. Meanwhile, I will try to help you with OCF, if I could find something. UPD: What I have found so far:
So ... I guess that for OCF we would need to find a way to pass the decoder hook ... maybe I would like to open PR to erlavro repo ✌🏼 |
|
Hi, i've added tests for this issue. Do you think it's OK ? |
| defp payment_schema do | ||
| {:ok, schema} = Schema.parse(payment_json_schema()) | ||
| %{schema | id: nil, version: nil} | ||
| end | ||
|
|
||
| defp payment_json_schema do | ||
| ~s({"namespace":"io.confluent","name":"Payment","type":"record","fields":[{"name":"id","type":"string"},{"name":"amount","type":"double"}]}) | ||
| ~s({"namespace":"io.confluent","name":"Payment","type":"record","fields":[{"name":"id","type":"string"},{"name":"amount","type":["null","double"]}]}) |
There was a problem hiding this comment.
Since you already introduce null_payment_payload, null_payment_message, I would suggest going with another schema (most of the things already different). It will also allow us to avoid confusion from the logical side (nullable amount).
| @@ -154,17 +165,26 @@ defmodule Avrora.Codec.PlainTest do | |||
|
|
|||
| defp payment_payload, do: %{"id" => "00000000-0000-0000-0000-000000000000", "amount" => 15.99} | |||
|
|
|||
| defp null_payment_payload, | |||
There was a problem hiding this comment.
Let's introduce another schema with a more logical null in it. Also, may I suggest to make null as a postfix, like ..._payload_with_null
| @@ -106,6 +112,11 @@ defmodule Avrora.Codec.PlainTest do | |||
| assert encoded == payment_message() | |||
| end | |||
|
|
|||
| test "when payload with null value is matching the schema and schema is usable" do | |||
| {:ok, encoded} = Codec.Plain.encode(null_payment_payload(), schema: payment_schema()) | |||
There was a problem hiding this comment.
To be consistent, can you make 1 blank line right before the assertion (like you did in the case above)
|
@matreyes Hey, how are you doing? Finally, my PR was merged and now in OCF you have access to the same set of decoder options as for binary decoder klarna/erlavro@a716c41 Also, I think for now I would like to keep the major version 0, so it's better to have this behavior optional as on by default, but with an ability to be turned off |
|
@matreyes If you don't mind I will continue this PR from now on. Thanks for your contribution 🤝 |
Strech
added
the
enhancement
|
Im really sorry, have been thought weeks. Im happy if you can continue.
thanks for your great work.
Matías Reyes
… El 16-11-2020, a la(s) 11:34, Sergey Fedorov ***@***.***> escribió:
@matreyes If you don't mind I will continue this PR from now on. Thanks for your contribution 🤝
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Hi!, Great work with Avrora!
Erlavro recommends to use hooks to convert the :null value (erlang) to nil for elixir.
I've added this to the default hook for Plain codec because it should be the default behaviour, but I couldn't find how to implement the hook on OCF.
It breaks the APIs, so if you approve it, it should go as major version.
Best