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

[Feature] - Allow importing the object from string in the factory #179

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

tarsil
Copy link
Collaborator

@tarsil tarsil commented Oct 3, 2023

Checklist

  • The code has 100% test coverage
  • The documentation was properly created or updated (if applicable) following the correct guidelines and language.
  • You branch out from the V2 tag and the V2 tag was properly rebased from main.

@ShahriyarR, today I was using this amazing new feature and I came across an idea to apply on the top of this and grow your initial functionality. Allowing to pass a string <module>.<object> type and delegate the import to the Factory itself. This way we can also avoid importing a lot of objects on the top of each needed file.

I ran a PoC and added some tests for you to see what I mean. In the end, it is adding an extra alternative on the top of the already existing functionality.

Something like this:

from esmerald import Factory, Include, Inject

route_patterns = [
    Include(
        "/api/v1",
        routes=[
            Include("/accounts", namespace="accounts.v1.urls"),
            Include("/articles", namespace="articles.v1.urls"),
            Include("/posts", namespace="posts.v1.urls"),
        ],
        interceptors=[LoggingInterceptor],  # Custom interceptor
        dependencies={
            "user_dao": Inject(Factory("myapp.accounts.daos.UserDAO")),
            "article_dao": Inject(Factory("myapp.articles.daos.ArticleDAO")),
            "post_dao": Inject(Factory("myapp.posts.daos.PostDAO")),
        },
    )
]

What do you think? Your idea for the factory was game changing here and I'm already using it.
@PeterChain what do you think about this too?

@tarsil tarsil changed the base branch from v2 to main October 3, 2023 20:37
@tarsil tarsil changed the base branch from main to v2 October 3, 2023 20:38
@ShahriyarR
Copy link

ShahriyarR commented Oct 4, 2023

I really like the idea, quite clean and neat ) I need to take a look at implementation, but the idea is powerful.

It can be further improved to have something called "automated wiring" to wire up dependencies and dependants.
Say we have defined the dependency as "user_dao": Inject(Factory("myapp.accounts.daos.UserDAO")),

And some other modules wait for this dependency.
You define your wirings(dependants):

dependencies={
            "user_dao": Inject(Factory("myapp.accounts.daos.UserDAO")),
            "article_dao": Inject(Factory("myapp.articles.daos.ArticleDAO")),
            "post_dao": Inject(Factory("myapp.posts.daos.PostDAO")),
        },
        
   wired = {
   "modules": ["myapp.accounts.views", "myapp.users.views"],
   "packages": ["somepackage"]
   }

Now in the "myapp.accounts.views" if you use something like global Provider: Provide["user_dao"] it will take it from dependencies and use, it again without any imports. But maybe we have already this idea in place.

@tarsil
Copy link
Collaborator Author

tarsil commented Oct 4, 2023

Is the wired also on every level? Hm, I would need some more details on this with more precise examples as I'm not entirely sure if I'm understanding. Maybe this is for another PR specifically?

@ShahriyarR
Copy link

Definitely, another PR will be a much better fit here.
For this PR, it looks awesome)

@tarsil
Copy link
Collaborator Author

tarsil commented Oct 4, 2023

Definitely, another PR will be a much better fit here. For this PR, it looks awesome)

Perfect! We can even open a discussion and go from there. Debate on this possibility. I would like to know and understand more. In the meantime, I will be merging this one.

@tarsil tarsil merged commit 1606fd7 into v2 Oct 4, 2023
9 checks passed
@tarsil tarsil deleted the feature/factory_by_import branch October 4, 2023 10:22
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