search

Strech / avrora

A convenient Elixir library to work with Avro messages, schemas and Confluent® Schema Registry
https://hexdocs.pm/avrora
MIT License
97 stars 33 forks source link

Make the custom Client configurable #91

Closed juanperi closed 3 years ago

juanperi commented 3 years ago

Closes #90

I've taken a stab at adding runtime configuration to custom Clients. I've not added any docs yet, as i'm not sure if you will like my approach or not.

Strech commented 3 years ago

Hey @juanperi thanks for your contribution. Yes the general idea is exactly like you have proposed 👍🏼

Unfortunately, implementation is sub-optimal due to several things:

  1. A new public method in the wrong spot (Client should know nothing about Config implementations)
  2. Module is passed to the options, but it's known in compilation time as module variable
  3. The get_config method is hard to understand, which config goes first, module or macros?
  4. A minor thing, but I'm not a big fan of programming in tests. It's hard to read, hard to understand, and easy to make a mistake. I would rather have fewer tests, but good ones.

I'm about to implement and release it this week, just in case.

juanperi commented 3 years ago

hey, I've updated the code with your suggestions 1-3, but feel free to close this PR to follow your approach I've done it because i'll be using the fork I've created until you release the new version.

Thank you for your review anyways!

juanperi commented 3 years ago

BTW, I just realized that If I use otp_app to be able to configure the client at runtime, I cannot use schemas_path anymore to set the fixtures for tests in the tests namespace, as the existance of OTP app converts the schemas path relative to the app folder. Which would mean that my test fixtures must live in the priv folder, and they will end up in the release.

juanperi commented 3 years ago

I've added a new commit, just to expose my problem with the schemas_path. I do understand that changing the meaning of the field (expanding vs not expanding) would be a breaking change though

Strech commented 3 years ago

Hey @juanperi a heads up. I've prepared PR #92 with some compile-time optimizations for runtime configuration. I'm planning to release it in a few hours today 👏

Strech commented 3 years ago

I would like to say thank you @juanperi for your support, effort, and healthy conversation around this enhancement. I've released v0.23.0 with all improvements 🍾