Skip to content

Conversation

@ilyapuchka
Copy link
Collaborator

To register factories/instances it's now possible to use up to six runtime arguments. Clients can extend this number on demand.

@AliSoftware Please review it again. There are few changes that I want to note:

  • I decided to remove method that registers factory that accepts tag as parameter, cause this case is covered with runtime arguments. Though I'm sure tags should stay, but they should be not used inside factories.
  • Adding methods with generic arguments introduces ambiguity with resolve method that accepts only tag parameter, so I had to make tag parameter named.

Copy link
Owner

Choose a reason for hiding this comment

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

I was confused at first to see that we stored the factory closure in the DefinitionKey. I now understand that in fact you store the factory signature / Type in there, to store the expected runtime argument types.

So here we probably need to find a different name for this property, to make it less confusing with the other places when we use the term factory which expect an actual closure, while in DefinitionKey that's the closure type, not the closure itself.
I only understood that when I saw your let key = DefinitionKey(protocolType: T.self, factory: F.self, associatedTag: tag) here, seeing that you used F.self there.

Simply renaming this to factoryType or factorySignature or whatnot would make things clearer I think.

@AliSoftware
Copy link
Owner

Adding methods with generic arguments introduces ambiguity with resolve method that accepts only tag parameter, so I had to make tag parameter named.

I'm OK with that, but I think maybe we should make it more consistent throughout the API. Probably make the tag parameter always a named parameter everywhere (even on methods not using runtime arguments) would keep consistency while also avoiding people to confuse the usage between a call with the tag and a call with one runtime argument.

@AliSoftware
Copy link
Owner

@ilyapuchka Thanks for this promising work!

I only reviewed it directly on GitHub for now, looks OK so far but will have to take another look and open the code in Xcode to properly check it out and review it better. Will keep you posted.

@AliSoftware
Copy link
Owner

  • Don't forget to add documentation in the README for those use cases. (I think the README can be improved and better organized to present the core concepts of Dip, btw)
  • Don't forget the CHANGELOG.md as well
  • Are you planning to add a Sample Project / Playground demonstrating usage of those Runtime Arguments in this PR, or do you plan to do it in a separate PR later?

@ilyapuchka
Copy link
Collaborator Author

Thanks for review, @AliSoftware, agree with all comments.
I was planning to add playground with all current features a bit later in separate PR.

@AliSoftware AliSoftware self-assigned this Nov 5, 2015
@ilyapuchka ilyapuchka force-pushed the feature/runtime-args branch from cb86c94 to 021906c Compare November 5, 2015 13:19
@ilyapuchka ilyapuchka force-pushed the feature/runtime-args branch from 021906c to b5fca0a Compare November 5, 2015 17:13
@ilyapuchka ilyapuchka force-pushed the feature/runtime-args branch from 222ce35 to f1c7c52 Compare November 7, 2015 11:45
@AliSoftware
Copy link
Owner

I wonder why sometimes Travis fails the compilation at the "▸ Touching DipTests.xctest" stage 😕
I just restarted Travis builds and they all pass now 👍 .

I promise I'll check this out tomorrow and hopefully we can finally merge it 😉

@ilyapuchka ilyapuchka force-pushed the feature/runtime-args branch from 11f18d4 to 9fc63ff Compare November 8, 2015 00:28
@ilyapuchka ilyapuchka force-pushed the feature/runtime-args branch from 9fc63ff to 2ea5e17 Compare November 8, 2015 00:31
@ilyapuchka
Copy link
Collaborator Author

Done. Also fixed some documentation slightly.
Would be cool if we can merge it. There are bunch of other cool features waiting for their PRs =)

@ilyapuchka
Copy link
Collaborator Author

I think this thread may address Travis issue - travis-ci/travis-ci#4725

@AliSoftware
Copy link
Owner

Ah good point on the Travis Thread, will keep an eye on it

(So it doesn't risk to be included by mistake to one of the project target, and to avoid confusing the user opening the Sample app)
@AliSoftware
Copy link
Owner

Perfect 👌

Finally, I have time to merge this, sorry for the delay!

AliSoftware added a commit that referenced this pull request Nov 8, 2015
Added support for up to six runtime arguments
@AliSoftware AliSoftware merged commit 3ccbff4 into develop Nov 8, 2015
@AliSoftware AliSoftware deleted the feature/runtime-args branch November 8, 2015 20:56
@AliSoftware
Copy link
Owner

(Note: you'll probably want to rebase your feature/playground on top of develop)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants