New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CIS-1238] Remove AnyXXXXControllerDelegate
Boilerplate Code
#1564
[CIS-1238] Remove AnyXXXXControllerDelegate
Boilerplate Code
#1564
Conversation
Generated by 🚫 Danger |
Sources/StreamChat/Controllers/ChannelListController/ChannelListController+Combine.swift
Outdated
Show resolved
Hide resolved
AnyXXXXControllerDelegate
boilerplate codeAnyXXXXControllerDelegate
Boilerplate Code
For some reason, without this the controller tests are not passing. Switching to `class` also make all tests failing still.
Codecov Report
@@ Coverage Diff @@
## main #1564 +/- ##
==========================================
- Coverage 87.87% 87.82% -0.05%
==========================================
Files 232 232
Lines 11029 10731 -298
==========================================
- Hits 9692 9425 -267
+ Misses 1337 1306 -31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Sources/StreamChat/Controllers/ChannelController/ChannelController.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of open stuff, but looks solid!
Sources/StreamChat/Controllers/ChannelController/ChannelController.swift
Outdated
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/ChannelController.swift
Outdated
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ConnectionController/ConnectionController.swift
Outdated
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/ChannelController.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple more points from me
Sources/StreamChat/Controllers/ChannelController/ChannelController_Tests.swift
Outdated
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/ChannelController.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
* Refactor `MulticastDelegate` * Change all controllers to the new `MulticastDelegate` * Remove All `AnyXXXControllerDelegate` boilerplate code * New `MulticastDelegate` Tests * Remove unnecessary `mutating` keyword from `MulticastDelegate` * Add back support for the main Delegate in `MulticastDelegate` * Set all functions to mutating on `MulticastDelegate` For some reason, without this the controller tests are not passing. Switching to `class` also make all tests failing still. * Fix MessageDTO_Tests * Add tests to verify there are no retain cycles * Add replace API to `MulticastDelegate` to replace the additional delegates * Deprecate `setDelegate()` * Update CHANGELOG.md * Improve deprecated `setDelegate` implementation * Remove `removeAll()` API from `MulticastDelegate` * Change the `replace` API with `set(additionalDelegates:)` for consistency * Move all deprecations to Deprecations.swift file * Remove TestGenericDelegate leftover from ExtraData
🔗 Issue Link
CIS-1238
🎯 Goal
Remove the
AnyXXXXControllerDelegate
boilerplate code, which complicates a lot when adding or creating delegates for our controllers in the Low-Level SDK. Since we don't have the generic ExtraData anymore, this boilerplate code is not needed.🛠 Implementation
By refactoring the
MulticastDelegate
and making sure the delegates are weakly referenced, we can totally discard theAnyXXXXControllerDelegate
implementations.🧪 Testing
A QA Run should be made to make sure nothing was broken.
@b-onc should also make sure the current implementation is correct, and it is compared with the old one. (I'm still not sure why we needed a "main" delegate before, but if this is necessary, we can put this back with the current implementation)Update: After discussing with Bahadir, I added back the mainDelegate toMulticastDelegate
.🎨 Changes
N/A
☑️ Checklist