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

Remove container resolving #255

Merged
merged 8 commits into from
Sep 1, 2018
Merged

Conversation

josephmancuso
Copy link
Member

@josephmancuso josephmancuso commented Aug 29, 2018

This PR that will remove resolving via parameter and only support annotations 😢
I actually kept the possibility to resolve parameters but it's disabled by default. You can potentially enable it on a per project basis via something like this in your wsgi.py file:

app = App(resolve_parameters=True)

but it's disabled by default and all Masonite packages and core will only use annotations

@coveralls
Copy link

coveralls commented Aug 29, 2018

Coverage Status

Coverage increased (+0.6%) to 92.006% when pulling ba30290 on remove-container-resolving into d189e65 on develop.

@josephmancuso josephmancuso self-assigned this Aug 29, 2018
@josephmancuso josephmancuso added the next major Issue scheduled for the next major release label Aug 29, 2018
@josephmancuso josephmancuso added this to the Masonite 2.1b1 milestone Aug 29, 2018
@josephmancuso josephmancuso mentioned this pull request Sep 1, 2018
31 tasks
@josephmancuso josephmancuso added the beta-1 Labels the issue with which beta version this issue applies to label Sep 1, 2018
@josephmancuso josephmancuso merged commit 1e8584a into develop Sep 1, 2018
@josephmancuso josephmancuso deleted the remove-container-resolving branch September 1, 2018 15:36
@vaibhavmule
Copy link
Contributor

What was the reason to remove container resolving?

@josephmancuso
Copy link
Member Author

is not particularly "removed". It is disabled by default now. You can enable it on a per application basis.

The reasoning was that as there becomes more and more packages being developed, simply using a simple namespace like Logger can be overridden by other packages. This means when building a package you have to think about all the conflicting binding names.

Some packages have the ability to override other packages by simply overriding the name in the container.

@josephmancuso
Copy link
Member Author

in 2.1 and possibly 2.2, the correct way to bind things will likely be doing something like:

app.simple(Class())

which binds into the container using the class name, which there should only ever be one class of the same type at a time

@vaibhavmule
Copy link
Contributor

Thanks for information.

Make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-1 Labels the issue with which beta version this issue applies to next major Issue scheduled for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants