klarna / erlavro

Avro support for Erlang/Elixir (http://avro.apache.org/)
Apache License 2.0
131 stars 39 forks source link

Add OCF decoder hook #104

Closed Strech closed 4 years ago

Strech commented 4 years ago

Closes #103

I still have some concerns about how I extend existing internal interfaces for OCF (I've added hook to the end of the existing list of arguments), so I'm very open to hear an opinion and adjust the code

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 616


Totals Coverage Status
Change from base Build 610: 0.0%
Covered Lines: 1708
Relevant Lines: 1708

💛 - Coveralls
zmstone commented 4 years ago

hmm. if we change avro_binary_decoder:decode_stream/4 like this:

@@ -79,9 +79,11 @@ decode_stream(IoData, Type, StoreOrLkupFun) ->
 -spec decode_stream(iodata(), type_or_name(),
                     schema_store() | lkup_fun(), hook()) ->
         {avro:out(), binary()}.
-decode_stream(IoData, Type, StoreOrLkupFun, Hook) ->
-  do_decode(IoData, Type, avro_util:ensure_lkup_fun(StoreOrLkupFun),
-            avro:make_decoder_options([{hook, Hook}])).
+decode_stream(IoData, Type, StoreOrLkupFun, Hook) when is_function(Hook) ->
+  decode_stream(IoData, Type, StoreOrLkupFun,
+                avro:make_decoder_options([{hook, Hook}]));
+decode_stream(IoData, Type, StoreOrLkupFun, Opts) when is_map(Opts) ->
+  do_decode(IoData, Type, avro_util:ensure_lkup_fun(StoreOrLkupFun), Opts).

we should be able to pass Opts down to avro_binary_decoder otherwise all other map fields (except for hook) provided by the caller are discarded.

Strech commented 4 years ago

we should be able to pass Opts down to avro_binary_decoder otherwise all other map fields (except for hook) provided by the caller are discarded.

Yes, I thought about the number of options for OCF being too small in comparison to decoder_options from the binary decoder. But I do pass only Hook (not all options), but I think it makes sense to pass all the decoder options because then I can re-use other functions from avro module, like make encoder and so on.

Thanks for the suggestion and let me try it!

Strech commented 4 years ago

@zmstone I adjust the code and in addition, I expand typespec of decode_stream/4 to reflect decoder_options

Strech commented 4 years ago

@zmstone All the pointed suggestions/issues were applied

zmstone commented 4 years ago

Great! Ping @mikpe @jesperes for approval & merge.

mikpe commented 4 years ago

The end result looks pretty good, but it's a lot of back and forth in the commits, which I'd rather avoid. Care to squash to a single commit?

Strech commented 4 years ago

@mikpe Sure, I can make a single commit, no problem

UPD: squash done 📦

Strech commented 3 years ago

@mikpe Hola 👋🏼 Can I ask when do you plan to release the next version with these changes?

mikpe commented 3 years ago

@Strech I can push a new tag tomorrow.

Strech commented 3 years ago

@mikpe Awesome! Thanks ❤️

mikpe commented 3 years ago

Updated changelog and tagged 2.9.3.

Strech commented 3 years ago

@mikpe Thanks a lot 🤝 🎉

Strech commented 3 years ago

@mikpe Sorry for the disruption, but I've noticed that the latest version on a https://hex.pm is 2.9.0, may I also ask to publish the 2.9.3 release? It will be super nice 🤗

mikpe commented 3 years ago

2.9.3 should be on hex now.

Strech commented 3 years ago

@mikpe Thanks again, much appreciated! 🚀