-
Notifications
You must be signed in to change notification settings - Fork 143
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
Ability to pass arguments #62
base: master
Are you sure you want to change the base?
Conversation
Thanks @webslesar for the pull request👍 I'll check it after merging #46 to avoid conflict. |
Can you add unit tests? |
Yes |
@yoichitgy Any plans for this? |
+1 for this. Would love to have this ability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long to review this.
In general it is heading in the right direction 👍 , we just need to remove the repeated code and clean up the API. If you need some help setting up code generation, feel free to ask 😉
And as @yoichitgy pointed out earlier, adding unit tests to such nontrivial functionality would be very useful.
name: "Animals", bundle: nil, container: container) | ||
let firstArg: Int = 5 | ||
let secondArg: SomeValue = SomeValue() | ||
let catController = sb.instantiateViewController(withIdentifier: "SomeIdentifier", arg1: firstArg, arg2: secondArg) as! AnimalViewController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more consistent to have API similar to Swinject, i.e.
instantiateViewController(withIdentifier: "SomeIdentifier", arguments: firstArg, secondArg)
|
||
extension SwinjectStoryboard { | ||
|
||
#if os(iOS) || os(tvOS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to extend this for OSX
as well
for child in viewController.childViewControllers { | ||
injectDependency(to: child) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of repeated code between methods with different arguments. Can we reuse this code somehow? Alternatively we can generate this similarly to how it is done in Swinject (see. Container.Arguments.erb)
} | ||
|
||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this file should probably be generated (see. Resolver.erb)
@@ -8,7 +8,7 @@ Pod::Spec.new do |s| | |||
s.homepage = "https://github.com/Swinject/SwinjectStoryboard" | |||
s.license = 'MIT' | |||
s.author = 'Swinject Contributors' | |||
s.source = { :git => "https://github.com/Swinject/SwinjectStoryboard.git", :tag => s.version.to_s } | |||
s.source = { :git => "https://github.com/SeductiveMobile/SwinjectStoryboard.git", :tag => s.version.to_s } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…toryboard # Conflicts: # Cartfile
Fixed the issue of arguments capturing while using 'storyboardInitCompletedArgs' family methods. |
# Conflicts: # Cartfile.resolved # SwinjectStoryboard.podspec
With this improvements we can add ability to pass 3 arguments