CombineCommunity / RxCombine

Bi-directional type bridging between RxSwift and Apple's Combine framework
MIT License
1.03k stars 86 forks source link

Update Observable+Combine #1

Closed jdisho closed 5 years ago

jdisho commented 5 years ago

First of all, thank you for RxCombine @freak4pc! 🎉

In this PR, I changed the implementation in Observable+Combine. Instead of exposing a computed property, publisher, I expose a method, asPublisher, that converts an Observable to a AnyPublisher.

Following the similar idea as here.

What do you think?

freak4pc commented 5 years ago

Hey @jdisho - thank you so much for this repo's first PR - very exciting!! :))

The naming was not accidental - I chose the naming conventions of Combine for Combine extesions, and the naming conventions of RxSwift to RxSwift extensions.

asObservable() is what Rx consumers will be used to,

While in Combine, it seems you would use textField.publisher to get a Publisher of String events (it was shown in one of the WWDC sessions) - so that seems to be the naming convention used there.

freak4pc commented 5 years ago

Another thought is - we can add asPublisher() as an alias just to support both, but I'm still not 100% sure of it. WDYT @jdisho ?

jdisho commented 5 years ago

Hey @freak4pc, thank you for the explanation. 🥇

To me asPublisher seems more suitable then publisher. _ = observable.asPublisher() tells that we are converting an observable to a publisher, while _ = observable.publisher tells me that publisher is a property of the observable and this may seem confusing.

However, I am fine if we support both. 😊

freak4pc commented 5 years ago

Hey there:) As much as I respect your opinion- I’ll always go for existing conventions when compared to a personal preference.

Let me think about it a bit and I’ll let you know in a bit.

Thanks ! :)

freak4pc commented 5 years ago

Decided to add the alias - Done in https://github.com/freak4pc/RxCombine/commit/289586b28776d119808d83d53eac18ce180bed4f

Gotta say, It's gonna take a while to get used to implicit returns 😆

jdisho commented 5 years ago

Awesome! Thank you for your time 😄

Sent with GitHawk

freak4pc commented 5 years ago

Thanks for the suggestion @jdisho 🙏