Skip to content

Conversation

robha141
Copy link
Contributor

Added new AsyncContainer, with async registration and resolution of dependencies.

@robha141 robha141 self-assigned this Dec 17, 2024
@robha141 robha141 marked this pull request as ready for review December 19, 2024 10:38
Copy link
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, I apologize for request changes, but I just wanna you double check the solution as well. There are many similarities in all those typealiases for "factories" but some of the types are not very understandable - although it's async copy paste of existing.
I know we're adding new "feature" without breaking changes, at the same time we created async DI for exploring swift6 complete concurrency check really fast way, therefore I'm for double check.

@robha141 robha141 requested a review from cejanen January 7, 2025 07:46
ipek
ipek previously approved these changes Jan 8, 2025
Copy link
Contributor

@ipek ipek left a comment

Choose a reason for hiding this comment

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

Very handy! I have just a few documentation and tests comments.

Copy link

@cernym46 cernym46 left a comment

Choose a reason for hiding this comment

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

I just looked at it briefly and left one thought. I'll do more throught review once I have more time. 😊

Copy link
Member

@DanielCech DanielCech left a comment

Choose a reason for hiding this comment

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

@robha141 Kudos. Impressive work. Please consider the change in the folder structure - maybe we can put Sync and Async variants of code into separate subfolders. What do you think?

And maybe silly question. If I have the same dependency that I need in both sync and async contexts, should I have two registrations? Can we somehow unify the interface as @cernym46 suggested?

@DanielCech
Copy link
Member

Maybe we can put Sync and Async variants of code into separate subfolders. What do you think?

I made the changes in the structure. Those changes are rather cosmetic but they allow to divide of synchronous, asynchronous, and common parts of the code.

ipek
ipek previously approved these changes Feb 17, 2025
Copy link
Contributor

@ipek ipek left a comment

Choose a reason for hiding this comment

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

Great job! I didn't find anything that would have effect on the usage. Just few naming/documentation notes and one nitpicking edgecase.

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

🍚

@cejanen cejanen merged commit aa5e4da into develop Feb 19, 2025
1 check failed
@cejanen cejanen deleted the feat/rob-async-init branch February 19, 2025 14:51
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.

5 participants