Skip to content
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

Add OCF decoder hook #104

Merged
merged 1 commit into from Nov 12, 2020
Merged

Add OCF decoder hook #104

merged 1 commit into from Nov 12, 2020

Conversation

Strech
Copy link
Contributor

@Strech Strech commented Nov 8, 2020

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
Copy link

coveralls commented Nov 8, 2020

Pull Request Test Coverage Report for Build 616

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

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

💛 - Coveralls

src/avro_ocf.erl Outdated
@@ -177,6 +195,11 @@ make_block(Header, Objects) ->
, Header#header.sync
].

-spec make_decoder_opts(codec_opts()) -> decoder_opts().
make_decoder_opts(Options) ->
DefaultOptions = [{hook, ?DEFAULT_DECODER_HOOK}],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe create a map here, and use maps:merger/2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think for now it should be simplified and not copy-pasted as I did, will change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed proplist to map also in the spectype

@zmstone
Copy link
Contributor

zmstone commented Nov 9, 2020

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
Copy link
Contributor Author

Strech commented Nov 10, 2020

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
Copy link
Contributor Author

Strech commented Nov 10, 2020

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

src/avro_ocf.erl Outdated Show resolved Hide resolved
src/avro_ocf.erl Outdated Show resolved Hide resolved
src/avro_ocf.erl Outdated
-spec decode_blocks(lkup(), avro_type(), avro_codec(),
binary(), binary(), [avro:out()]) -> [avro:out()].
decode_blocks(_Lkup, _Type, _Codec, _Sync, <<>>, Acc) ->
binary(), binary(), [avro:out()], decoder_options()) -> [avro:out()].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is too long.

@Strech
Copy link
Contributor Author

Strech commented Nov 10, 2020

@zmstone All the pointed suggestions/issues were applied

@zmstone
Copy link
Contributor

zmstone commented Nov 10, 2020

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

@mikpe mikpe requested a review from a team November 10, 2020 15:29
@mikpe
Copy link
Member

mikpe commented Nov 10, 2020

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
Copy link
Contributor Author

Strech commented Nov 10, 2020

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

UPD: squash done 📦

* Add new public function avro_ocf:decode_binary/2
* Add new public function avro_ocf:decode_file/2
* Extend function interface for avro_binary_decoder:decode_stream/4
@mikpe mikpe merged commit e04f4a4 into klarna:master Nov 12, 2020
@Strech Strech deleted the add-ocf-decoder-hook branch November 12, 2020 14:43
@Strech
Copy link
Contributor Author

Strech commented Nov 16, 2020

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

@mikpe
Copy link
Member

mikpe commented Nov 16, 2020

@Strech I can push a new tag tomorrow.

@Strech
Copy link
Contributor Author

Strech commented Nov 16, 2020

@mikpe Awesome! Thanks ❤️

@mikpe
Copy link
Member

mikpe commented Nov 17, 2020

Updated changelog and tagged 2.9.3.

@Strech
Copy link
Contributor Author

Strech commented Nov 17, 2020

@mikpe Thanks a lot 🤝 🎉

@Strech
Copy link
Contributor Author

Strech commented Nov 17, 2020

@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
Copy link
Member

mikpe commented Nov 18, 2020

2.9.3 should be on hex now.

@Strech
Copy link
Contributor Author

Strech commented Nov 18, 2020

@mikpe Thanks again, much appreciated! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OCF decoder hook
4 participants