Feat: Support for multiple arguments (1/3)#22
Conversation
# Conflicts: # Sources/Protocols/Registration/Async/AsyncDependencyRegistering.swift # Sources/Protocols/Registration/Sync/DependencyWithArgumentAutoregistering.swift # Sources/Protocols/Registration/Sync/DependencyWithArgumentRegistering.swift
cejanen
left a comment
There was a problem hiding this comment.
well, too many texts which are boilerplate - specially all those DISCUSSION in comments - it can be there just once for every type - resolver etc...
second - there are some changes which may be breaking for app using the older version - would be great to have tutorial how to update to new version and test it first.
For tests - I would like to see more tests towards the thread safety - register/resolve to container from multiple threads...
|
@cejanen Thanks for your thorough review. I fixed the reported issues. I think now it is ready for review again. Regarding additional tests of the thread-safe behavior - I'd like to focus on this in the next PR that uses the Testing framework. |
|
Can we fix CI as well? Not necessarily this PR but "asap" cc @TParizek @DanielCech |
@DanielCech To fix the CI, please change the runner label in this project from |
|
@DanielCech The PR looks great. Thanks for all the updates and fixing the CI issue. My only last feedback is about the scripts folder - do you think they should be commited in the repository? I understand they add some value, but having them in the repository adds extra maintenance that we would need to do over time. |
I agree. I removed the script and made necessary changes to fix CI. |
This PR is built on top of #21