Release 2.0 #11

tkohout · 2017-01-29T11:53:51Z

Updated Swinject to 2.0
Migrated to new swinject
Resolved Mistaking too many initialiser arguments for a tuple #2 Added warnings when
- fails to resolve service with optional dependencies
- fails to resolve service with implicitly unwrapped dependencies
- fails to resolve service with more than maximum supported number of dependencies
- trying to register service with same type of dynamic arguments
Added assert tests Testing asserts #6
Added swift package manager Swift PM support? #9

…into separated files

# Conflicts: # Cartfile.resolved # Carthage/Checkouts/Nimble/Nimble.podspec # Carthage/Checkouts/Nimble/Nimble.xcodeproj/project.pbxproj # Carthage/Checkouts/Nimble/Sources/Nimble/Nimble.h # Sources/AutoRegistration.swift # bin/generate

…gistration into JiLiZART-swift-pm

yoichitgy · 2017-01-29T13:09:45Z

.travis.yml

+    - env: JOB="POD_LINT"
+      script:
+        - pod lib lint
+    - env: JOB="XCODE" DEST="OS=8.4,name=iPhone 5" SCHEME="SwinjectAutoregistration-iOS" SDK="iphonesimulator" ACTION="test"


Changing the device to iPhone 5s looks ok to fix the error on Travis with Nimble.
https://travis-ci.org/Swinject/SwinjectAutoregistration/jobs/196326139

DEST="OS=8.4,name=iPhone 5s"

yoichitgy · 2017-01-30T23:49:50Z

It looks good to run Carthage command with --use-submodules option to add dependencies as git submodules not to commit files of Swinject, Quick and Nimble to this repository. It's easier to review a pull request without those library files.

@tkohout, using the option is ok for you?

jakubvano · 2017-02-01T08:40:22Z

👍 Good job guys on 2.0 release!

@yoichitgy Is there a benefit of having carthage dependencies included as submodules and subprojects as opposed to just linked frameworks (not pushed in repo)? I suspect that when you have both Swinject and SwinjectAutoregistration in project Cartfile, this causes carthage effectively build Swinject twice (once as a carthage dependency, once as a subproject in SwinjectAutoregistration), and also might be causing Swinject#184.

yoichitgy · 2017-02-02T12:29:13Z

Thanks @jakubvano for pointing out the issue👍👍👍 It looks good not to commit submodules.

Or maybe users should add only SwinjectAutoregistration to Cartfile to install both SwinjectAutoregistration and Swinject??? For example, installation guide of ReactiveCocoa tells to add only ReactiveCocoa to Cartfile to install with ReactiveSwift. Are you familiar with this idea?

I'm going to try some experiments.

jakubvano · 2017-02-03T07:57:07Z

I get what you mean, but this approach would not cover the case when you use more than one Swinject extension (e.g. SwinjectAutoregistration and SwinjectStoryboard).

yoichitgy · 2017-02-05T02:51:39Z

I agree. My approach doesn't fit in the case using more than a Swinject extension. I'm going to have some experiment as @jakubvano said.

yoichitgy · 2017-02-11T12:45:29Z

I started adding depending projects to a project because of this issue:
Swinject/Swinject#40

For example, Quick and Nimble were added as projects, not prebuilt frameworks. In the same way, I added Swinject project to SwinjectStoryboard and SwinjectPropertyLoader.

The original issue looked already resolved, so we have no reasons to add depending projects. As @jakubvano told, we can just add frameworks built by Carthage, and depending projects can be removed from the repository.

yoichitgy · 2017-02-11T15:01:53Z

I made the migration to prebuilt frameworks in the following branch. I think it should be in another pull request, so that I'm going to just review this current pull request.
https://github.com/Swinject/SwinjectAutoregistration/tree/migrate-to-prebuilt-frameworks

yoichitgy

As my review, I added a comment about warnings to CheckResolved.swift.

Also I found unit test it("throws assertion when same type arguments are passed") failed only in case of tvOS target.

yoichitgy · 2017-02-11T15:16:56Z

Sources/CheckResolved.swift

+   let unresolved = ( [a] as [Any?] ).filter { $0 == nil }
+   if unresolved.count > 0 {
+       let warningsMessage = warnings(forInitializer: initializer).map { "\($0.message)\n" }.joined()
+       fatalError("SwinjectAutoregistration: Resolution failed.\n\(warningsMessage)Unresolved service: \(unresolvedService(a)!)\nInitializer: (\(A.self)) -> \(Service.self)")


If this is just a warning message, shouldn't this be print or NSLog? The other warnings in this file are the same.

Well the warnings describe what caused that the service was unresolved: Something like:

⚠ Autoregistration of implicitly unwrapped optional dependency (DependencyA)! is not supported. Use regular `register` method.

And if i would just print them before the fatalError they might have been overlooked

The tvOS throw assertion fail is fixed

Thank you for fixing the tvOS issue!

[nits] This is just a tiny thing, but if warningMessage is used for fatalError naming the parameter errorMessage sounds less misunderstandable. Maybe related parameter names also sound good with "error"😉

Thanks, I refactored the code to address your notes.

yoichitgy

Thank you for accepting my feedback😃🐶☀️

yoichitgy · 2017-02-19T05:09:30Z

I think we can release v2.0.0 now.

My changes to migrate to prebuilt frameworks can be released as v2.1 or v2.0.1 because the changes are not considered as breaking changes.
https://github.com/Swinject/SwinjectAutoregistration/tree/migrate-to-prebuilt-frameworks

tkohout and others added 15 commits October 25, 2016 14:48

Updated podspec

ee05caa

Changed podspec to project structure

6206eda

Updated to newest Swinject

c859a78

Updated ResolverType to Resolver

1af3d7c

swift pm support

c963bc4

Added message when trying to resolve Optional dependency. Refactored …

c370898

…into separated files

Added useful warnings when resolution fails

e5e71fb

Clean up

44d7ac5

Added fatalError for same type dynamic arguments

4ff8813

Updated quick and nimble, added assertion tests

50c40ba

More checkouts, podspec fix

a6d5428

Readme update, version bump

1b1a2da

Merge branch 'swift-pm' of https://github.com/JiLiZART/SwinjectAutore…

3e292b9

…gistration into JiLiZART-swift-pm

Merge branch 'JiLiZART-swift-pm' into release2.0

dd2f97e

tkohout assigned yoichitgy and jakubvano Jan 29, 2017

tkohout added 3 commits January 29, 2017 13:52

Added pm tests, updated .travis.yml

41bc919

Podspec fix

60f1249

Added swift version for travis

4066693

yoichitgy reviewed Jan 29, 2017

View reviewed changes

tkohout added 6 commits January 29, 2017 14:57

Nimble matcher fix

0c4646b

Travis build fixes

db5fdd5

Moved tests for spm

68c58af

Added pod repo update

f7daa6f

Updated tests and code to support Linux

31c4d17

Removed linux support for warnings

f7a4d2e

yoichitgy reviewed Feb 11, 2017

View reviewed changes

tkohout added 2 commits February 12, 2017 22:22

Fixed throw assertion test fail

b18fcb7

Refactored warnings to resolution error

ccc65de

yoichitgy approved these changes Feb 19, 2017

View reviewed changes

yoichitgy merged commit 2cbec03 into master Feb 19, 2017

yoichitgy deleted the release2.0 branch February 19, 2017 05:06

yoichitgy mentioned this pull request Mar 8, 2017

Use prebuilt frameworks and remove code checked out by Carthage #12

Closed

yoichitgy mentioned this pull request Mar 15, 2017

Using SwinjectStoryboard with Swinject by adding the projects to an app project/workspace Swinject/Swinject#184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release 2.0 #11

Release 2.0 #11

tkohout commented Jan 29, 2017

yoichitgy Jan 29, 2017

yoichitgy commented Jan 30, 2017

jakubvano commented Feb 1, 2017 •

edited

yoichitgy commented Feb 2, 2017

jakubvano commented Feb 3, 2017

yoichitgy commented Feb 5, 2017

yoichitgy commented Feb 11, 2017 •

edited

yoichitgy commented Feb 11, 2017

yoichitgy left a comment

yoichitgy Feb 11, 2017

tkohout Feb 12, 2017

tkohout Feb 12, 2017

yoichitgy Feb 12, 2017

tkohout Feb 18, 2017

yoichitgy left a comment

yoichitgy commented Feb 19, 2017

Release 2.0 #11

Release 2.0 #11

Conversation

tkohout commented Jan 29, 2017

yoichitgy Jan 29, 2017

Choose a reason for hiding this comment

yoichitgy commented Jan 30, 2017

jakubvano commented Feb 1, 2017 • edited

yoichitgy commented Feb 2, 2017

jakubvano commented Feb 3, 2017

yoichitgy commented Feb 5, 2017

yoichitgy commented Feb 11, 2017 • edited

yoichitgy commented Feb 11, 2017

yoichitgy left a comment

Choose a reason for hiding this comment

yoichitgy Feb 11, 2017

Choose a reason for hiding this comment

tkohout Feb 12, 2017

Choose a reason for hiding this comment

tkohout Feb 12, 2017

Choose a reason for hiding this comment

yoichitgy Feb 12, 2017

Choose a reason for hiding this comment

tkohout Feb 18, 2017

Choose a reason for hiding this comment

yoichitgy left a comment

Choose a reason for hiding this comment

yoichitgy commented Feb 19, 2017

jakubvano commented Feb 1, 2017 •

edited

yoichitgy commented Feb 11, 2017 •

edited