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

Throwing factories and resolve dependencies block #32

Merged
merged 2 commits into from
Jan 10, 2016

Conversation

ilyapuchka
Copy link
Collaborator

With that PR we no more need to call container.resolve() with try! inside a factory or resolveDependencies block. Also convenient to use throwing constructors or any other throwing methods inside those blocks. Error will be catched by container, printed in console and then will be re-thrown.

Note: There is no way (at least for now) I can think of to be able to distinguish between throwing and not-throwing factories (thus the latest definition will override previous), except of duplicating all register/resolve methods, what I don't like at all cause it will double the number of methods in container. Taking in account that tag is an optional argument and we have support for 6 runtime arguments already (and upcoming multi-injection feature will ad 6 more methods) it will make API too much bloated.

@@ -186,28 +186,30 @@ public final class DependencyContainer {
Though before you do that you should probably review your design and try to reduce the number of dependencies.

*/
public func resolve<T, F>(tag tag: Tag? = nil, builder: F->T) throws -> T {
public func resolve<T, F>(tag tag: Tag? = nil, builder: F throws -> T) throws -> T {
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if the outer throws should be throws or rethrows?

http://robnapier.net/re-throws

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here rethrows will not compile cause this method throws error itself.

@AliSoftware
Copy link
Owner

Just make sure you double-check every time you use (f: … throws) -> throws if it's valid or if a rethrows might instead be used. But apart from that LGTM.

@AliSoftware
Copy link
Owner

Note: once you merge that, make sure to rebase #13 before merging it, as I'm wondering if this change might have impacts on the playground / example code you added there too, like changing calls using try there?

@ilyapuchka
Copy link
Collaborator Author

Looks like _resolve is the only place where rethrows is applicable, except ResolvedInstances.resolve where it is already used.

@AliSoftware
Copy link
Owner

👌

@ilyapuchka
Copy link
Collaborator Author

Good catch, these changes actually broke #13 😞 will try to figure out the fix

@ilyapuchka
Copy link
Collaborator Author

Fixed the issue, it is safe to merge this now 🎉

AliSoftware added a commit that referenced this pull request Jan 10, 2016
Throwing factories and resolve dependencies block
@AliSoftware AliSoftware merged commit d786eb1 into develop Jan 10, 2016
@ilyapuchka ilyapuchka deleted the feature/throwing-factories branch January 14, 2016 18:46
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