Skip to content

Feat: Support for multiple arguments (1/3)#22

Merged
DanielCech merged 56 commits intomasterfrom
feat/multiple-arguments
Mar 6, 2026
Merged

Feat: Support for multiple arguments (1/3)#22
DanielCech merged 56 commits intomasterfrom
feat/multiple-arguments

Conversation

@DanielCech
Copy link
Copy Markdown
Member

@DanielCech DanielCech commented Jan 13, 2026

  • One thing that I missed in the previous implementation of our DI library was support for more than one argument. I was planning the change for some time.
  • We were forced to create some temporary structures only for argument propagation. It is not bad practice, but sometimes using, for example, two arguments, is handy.
  • This PR introduces support for up to three arguments - it uses a new Swift feature: parameter packs
  • Code refactoring
  • New tests
  • Documentation
  • I switched from Bundler to Mise and improved the setup to match almost our mobile template. CI is running again.

This PR is built on top of #21

# Conflicts:
#	Sources/Protocols/Registration/Async/AsyncDependencyRegistering.swift
#	Sources/Protocols/Registration/Sync/DependencyWithArgumentAutoregistering.swift
#	Sources/Protocols/Registration/Sync/DependencyWithArgumentRegistering.swift
Copy link
Copy Markdown
Contributor

@cejanen cejanen left a comment

Choose a reason for hiding this comment

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

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...

Comment thread Documentation/Documentation.md Outdated
Comment thread Sources/Models/RegistrationIdentifier.swift
Comment thread Sources/Protocols/Registration/Sync/DependencyRegistering.swift Outdated
Comment thread Tests/Container/Sync/ArgumentTests.swift
@DanielCech DanielCech deleted the branch master January 14, 2026 14:18
@DanielCech DanielCech closed this Jan 14, 2026
@DanielCech DanielCech reopened this Jan 14, 2026
@DanielCech
Copy link
Copy Markdown
Member Author

@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.

@DanielCech DanielCech requested a review from cejanen January 15, 2026 11:13
@DanielCech DanielCech changed the base branch from feat/formatting to master January 15, 2026 14:11
@cejanen
Copy link
Copy Markdown
Contributor

cejanen commented Jan 16, 2026

Can we fix CI as well? Not necessarily this PR but "asap" cc @TParizek @DanielCech

@TParizek
Copy link
Copy Markdown
Contributor

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 ios-integrations to mobile-ci.

@TParizek
Copy link
Copy Markdown
Contributor

TParizek commented Mar 5, 2026

@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.

@DanielCech
Copy link
Copy Markdown
Member Author

@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.

Copy link
Copy Markdown
Contributor

@TParizek TParizek left a comment

Choose a reason for hiding this comment

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

📦

@TParizek TParizek dismissed cejanen’s stale review March 5, 2026 15:01

Feedback fixed

@DanielCech DanielCech merged commit 10d1769 into master Mar 6, 2026
2 checks passed
@DanielCech DanielCech deleted the feat/multiple-arguments branch March 6, 2026 12:12
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.

4 participants