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

Making typing.Type injectable. #28

Closed
nea89o opened this issue Jun 19, 2020 · 5 comments
Closed

Making typing.Type injectable. #28

nea89o opened this issue Jun 19, 2020 · 5 comments
Labels
new feature This issue or PR is about a new feature solved This issue is solved

Comments

@nea89o
Copy link
Contributor

nea89o commented Jun 19, 2020

Introduction

I am currently into an issue where i need the Type of all @injectable objects. These objects have a common supertype, but no empty __init__, so I can't just use Autowired(List[BaseType]). Concrete, this is a database project, and the model classes need to be registered so they can be created in the database.

This is kinda far fetched, and i could definitely see why this would be rejected, as database models are kind of the only usecase i coud think of, and it is kind of narrow.

Feature Request

Allowing the Autowired(Type[BaseType]) to pick up an injectable subclass of a BaseType, or Autowired(List[Type[BaseType]]) to get all injectable subclasses. Maybe this could be indicated by @injectable(type=True).

By submiting an issue I accept this project's
Code of Conduct. Any issue out of
these guidelines can be deleted at any moment without further explanation.

@roo-oliv roo-oliv added new feature This issue or PR is about a new feature triage This issue or PR is marked for triage labels Jun 19, 2020
@roo-oliv
Copy link
Owner

Hi @romangraef! Thank you for the issue with implementation suggestions details ♥️

Could you provide a minimal working example of how you're using injectable today? I fear I wasn't fully able to see it through. With a concrete example, it will be easier to spot the issue's root cause and think of the best way out of it.

@nea89o
Copy link
Contributor Author

nea89o commented Jun 19, 2020

I created a gist with a minimal (not) working sample of what i tried. In case you don't know peewee (a sql orm), the most important function in order to understand this gist should be engine.create_tables() which accepts an array of table classes, which it then creates in the database.

https://gist.github.com/romangraef/584f8c075143412dd0f681a5b189e440

@roo-oliv
Copy link
Owner

Thank you for the gist @romangraef. I've run it and it does give me an error due to the Type wrapper. I've removed the Type wrapper and just used tables: Autowired(typing.List[TableBase]) and it worked.

Sorry, I think that I didn't get it when you said you can't use Autowired(List[BaseType]), especially the "no empty __init__" thing. Can you clarify this for me?

@nea89o
Copy link
Contributor Author

nea89o commented Jun 19, 2020

Hmm, i thought you couldn't instantiate the tables objects without arguments, but i might have been wrong. Well that saved me that trouble, and I'm sorry for yours. It might still be an interesting feature to be able to inject the Type instead, but (as i said in my OP) there probably aren't that many use cases except for this specific one.

@roo-oliv
Copy link
Owner

Injectables mustn't depend on mandatory arguments. In case arguments are needed for instantiation one should declare an injectable_factory to correctly instantiate objects.

Currently, I don't see a real benefit in allowing wrapping types with Type. It is odd that one would want an object of type Type and most probably typing.Type isn't intended for that kind of usage.

I'm really glad that you solved your issue 🎉 Please, feel free to open other issues if you find it necessary or to post a question at StackOverflow with the python-injectable tag.

As the issue seems solved I'm closing it. If you believe that it should be reopened just leave a comment and I'll review it.

@roo-oliv roo-oliv added solved This issue is solved and removed triage This issue or PR is marked for triage labels Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This issue or PR is about a new feature solved This issue is solved
Projects
None yet
Development

No branches or pull requests

2 participants