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

Koin dsl #3966

Merged
merged 7 commits into from
Oct 3, 2023
Merged

Koin dsl #3966

merged 7 commits into from
Oct 3, 2023

Conversation

abelgardep
Copy link
Contributor

Koin now offer a new kind of DSL keyword that allow you to target a class constructor directly, and avoid to to have type your definition within a lambda expression.

Use koin constructor dsl to inject view models, repositories, data sources etc.. Its a little bit cleaner now, and no need to introduce get() get() get() over the whole module.

QA

@abelgardep
Copy link
Contributor Author

I think that Koin DSL makes it easier to define the dependencies. It removes some verbosity that we have in the standard way.

If you consider it useful, I can fix the conflicts. Otherwise, close the pull request 👍

@jesmrec @Aitorbp

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2023

We are still interested, will not be closed. As usual, time reduced. Thanks a lot.

EDIT: will ping you when conflicts must be solved to test/merge

@jesmrec jesmrec added the Sprint label Aug 25, 2023
@abelgardep
Copy link
Contributor Author

I'm not able to fix the conflicts because I don't have write access to the repository anymore 😢

I could try to cherry-pick and create a new PR from my fork though. Otherwise, you will need to fix the conflicts yourselves.

So up to you 👍

@jesmrec
Copy link
Collaborator

jesmrec commented Sep 4, 2023

We'll try to fix first. Thanks again for your engagement.

@michaelstingl
Copy link
Contributor

I don't have write access to the repository anymore 😢

Should be fixed 👍

@abelgardep abelgardep marked this pull request as ready for review September 10, 2023 20:32
@abelgardep
Copy link
Contributor Author

Rebased.

Little consideration: We can't inject over 10 dependencies using koin dsl. For example, that's why I kept TransfersViewModel the way it was.

@JuancaG05 JuancaG05 self-requested a review September 12, 2023 13:20
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Some tiny comments here @abelgardep 🍻

@JuancaG05 JuancaG05 self-assigned this Sep 19, 2023
@abelgardep
Copy link
Contributor Author

Changes applied and rebased @JuancaG05 🍻

Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution @abelgardep! 🚀

@JuancaG05 JuancaG05 merged commit e6a65c0 into master Oct 3, 2023
1 check passed
@JuancaG05 JuancaG05 deleted the koin_dsl branch October 3, 2023 06:54
@abelgardep abelgardep restored the koin_dsl branch October 5, 2023 18:26
@abelgardep abelgardep deleted the koin_dsl branch October 5, 2023 18:34
@jesmrec jesmrec removed the Sprint label Oct 6, 2023
jesmrec pushed a commit that referenced this pull request Oct 20, 2023
Aitorbp pushed a commit that referenced this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants