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

feat: Accept multiple apps configs #14

Merged
merged 9 commits into from
Mar 13, 2023

Conversation

pasm94
Copy link
Contributor

@pasm94 pasm94 commented Feb 26, 2023

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

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

@cpursley
Copy link
Owner

cpursley commented Mar 6, 2023

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

pasm94 commented Mar 7, 2023

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

@cpursley
Copy link
Owner

cpursley commented Mar 7, 2023

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

pasm94 commented Mar 8, 2023

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

cpursley commented Mar 8, 2023

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

pasm94 commented Mar 8, 2023

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

cpursley commented Mar 8, 2023

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

@pasm94
Copy link
Contributor Author

pasm94 commented Mar 12, 2023

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

@cpursley
Copy link
Owner

cpursley commented Mar 12, 2023 via email

@cpursley cpursley merged commit a9e6b9b into cpursley:master Mar 13, 2023
@cpursley
Copy link
Owner

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

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.

2 participants