search

cpursley / walex

Postgres change events (CDC) in Elixir
MIT License
282 stars 14 forks source link

feat: Accept multiple apps configs #14

Closed pasm94 closed 1 year ago

pasm94 commented 1 year ago

Hi!

I made this implementation with the aim of making Walex flexible for multiple configurations. The main use case would be Umbrella Apps, using different configs in each app and even different databases.

I did some tests, and it's working fine.

I also made some code changes following this popular style guide for elixir https://github.com/christopheradams/elixir_style_guide, and some other changes after running credo https://github.com/rrrene/credo on the project.

I know the change ended up changing the library quite a bit, and maybe even a new WalEx 2.0.0 version!? But I believe it can be very useful for several applications.

Please let me know if you find anything you think is wrong.

In the next few days I intend to start implementing tests in the application, and I think it makes sense to add to this PR, so I will ask you to wait any possible merge from this PR until I finished it :)

cpursley commented 1 year ago

Thanks! I'll try to review this in the next week or two.

cpursley commented 1 year ago

@pasm94

Thank you for the tests!

I'm not sure I love having to pass app_name around everywhere to use the helper functions. For example:

event(table_name, txn, app_name)

Is there a way we can set this up where app_name is optional for non-umbrella / single app scenarios situations - where the app_name is automatically picked up from the config? Or even not necessary at all for simpler setups?

pasm94 commented 1 year ago

Yes, I think it makes sense. Let me some days and I will improve it!

cpursley commented 1 year ago

Also, I'd love to have your feedback on the API for dealing with the events. I feel like it could be greatly simplified - maybe even a DSL type of thing in the future. Ditto on the config (having to pass in all the modules and subscriptions is a bit clumsy.

pasm94 commented 1 year ago

@cpursley thinking here, I'm considering that the ideal solution would be for the events/event modules to be __using__ macros. We will still using as today, just changing to use WalEx.Event, name: :my_app for umbrella apps or even only use WalEx.Event for single apps instead import functions. In this way we do not need to pass app_name in any function.

What do you think?

cpursley commented 1 year ago

I was also thinking about macros (thought that can dangerous) but haven't thought through how that might look yet.

I like your idea of use and is how a lot of libraries work.

pasm94 commented 1 year ago

Yes, I think it can simplify more things. I will work on it next days. I think it means we also do not need to pass modules and subscriptions on configs anymore, we can just pass in __using__ opts too

Edit: Actually not sure about this, need to think more about. Anyway, I will send a PR next days.

cpursley commented 1 year ago

Super! If you end up working on the Event API please start another branch separate from this multi-config one.

pasm94 commented 1 year ago

Hey! I did it, waiting for your review :)

cpursley commented 1 year ago

Super, I’ll take a look this week. Thank you.

On Sun, Mar 12, 2023 at 4:01 PM Paulo A. S. Müller @.***> wrote:

Hey! I did it, waiting for your review :)

— Reply to this email directly, view it on GitHub https://github.com/cpursley/walex/pull/14#issuecomment-1465286588, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJKYXKGT3QFDDFXHHT4F6DW3YTTJANCNFSM6AAAAAAVIEVQ4I . You are receiving this because you were mentioned.Message ID: @.***>

cpursley commented 1 year ago

Thank you @pasm94 - I tested this, merged and created a 2.0.0 release. I appreciate the help.