disposeBag: add a HasDisposeBag protocol by twittemb · Pull Request #41 · RxSwiftCommunity/NSObject-Rx

twittemb · 2017-08-28T13:57:06Z

In this commit, we try to conform to protocol oriented programming. It is more flexible to
introduce a DisposeBagable protocol so that every object that respect this one
can have a DisposeBag (NSObject does respect that protocol now)

ashfurrow

Looks good to me! Anyone from @RxSwiftCommunity/contributors available to review?

ashfurrow · 2017-08-28T14:15:10Z

+        get {
+            var disposeBag: DisposeBag!
+            doLocked {
+                let lookup = objc_getAssociatedObject(self, &AssociatedKeys.rxDisposeBag) as? DisposeBag


Interesting – I didn't expect associated objects to work with non-NSObject subclasses. You've verified this works?

I will add a Unit test for this

Good idea 👍

Done, there is a new Test for that. It works.

ashfurrow · 2017-08-28T14:15:19Z


 It'll work just like a property: when the instance is deinit'd, the `DisposeBag` gets disposed. It's also a read/write property, so you can use your own, too.

+If you want to add a DisposeBag to an Object that does not inherit from NSObject, you can also implement the protocol "DisposeBagable", and you're good to go. This protocol provides a default DisposeBag called rxDisposeBag.


Excellent addition to the readme 👍

Wondering on why we should call this rxDisposebag given the fact it's from RxSwift is rather implicit? what about just calling it disposeBag across the board?

Good point. I will fix that.

ashfurrow · 2017-08-28T14:15:39Z

+        let address2 = Unmanaged<AnyObject>.passUnretained(newDisposeBag as AnyObject).toOpaque()
+        XCTAssert(address1 == address2)
+    }
+}


Good test coverage, awesome!

freak4pc · 2017-08-28T14:25:35Z

My only gotcha with this is I really don't think this is an intuitive protocol name. Definitely an opinionated thing as naming is hard :)

Great implementation though!

twittemb · 2017-08-28T14:37:05Z

For the name, there is another possibility: HasDisposeBag
Is this better ?

freak4pc · 2017-08-28T14:38:31Z

What about Disposing (also possible Disposer but less common than using ing when able doesn't make english sense) ?

It's very tricky since naming conventions are hard in this sense :)

Opinions ? @RxSwiftCommunity/contributors

freak4pc · 2017-08-28T14:47:20Z

You should also fix CI :)

twittemb · 2017-08-28T14:47:27Z

I like the HasDisposeBag, because it is very explicit when you read the signature of the class that will implements it.

twittemb · 2017-08-28T14:49:11Z

CI is broken for the Demo app. It was already broken before I think.
As far I as I can tell, the demo app is just for Unit Testing. I think it could be removed since we can implement Unit Tests directly in the main project. Am I wrong ?

freak4pc · 2017-08-28T14:53:37Z

I'm definitely OK with this @twittemb .
Master is actually passing green so this seems to be related to the changes you made here

ashfurrow · 2017-08-28T14:54:16Z

Yeah, CI is broken for this PR but it's not your fault! Looks like you're a Carthage user right? We just need to run pod update NSObject-Rx inside the Demo directory. I can take care of that if this PR is otherwise good to go.

EDIT: I've updated the readme to have the CI badge display the latest master build, instead of just the latest build of any branch.

freak4pc · 2017-08-28T14:55:56Z

I think the two pending items are :

Having the associated var named disposeBag across the board instead of rxDisposebag and updating docs accordingly
Gather feedback from community regarding protocol name. Or if there is no feedback for a day or two just leave the current naming which is fine as well :)

twittemb · 2017-08-28T15:03:14Z

Ok. I've made the following changes:

the var is now named "disposeBag",
the protocols is now named "HasDisposeBag", it should be a more consensual choice.

freak4pc · 2017-08-28T15:05:42Z

Thanks for the disposeBag change. 💯

Regarding the Has prefix, I've never seen a naming convention where that is used for a protocol so I still think it would be best to wait for some feedback from other contributors. And if not we can perhaps just leave DisposeBagable.

HasDisposeBag feels a bit off perhaps? I guess i'm not religious about it but just not sure that's proper naming

ashfurrow · 2017-08-28T15:08:00Z

See this is why I liked having all protocols suffixed with "Type", but that went away in Swift 3 :\ I'm in favour of either – we can always change it later + add a typealias for the old name.

twittemb · 2017-08-28T15:09:38Z

I was inspired by the following article by Krzysztof Zabłocki, who is involved in the swift community.
http://merowing.info/2017/04/using-protocol-compositon-for-dependency-injection/

He uses the "Has..." to specify every kinds of Services a class can provide (in a DI context)

Another idea: It could be something like "DisposeBagProvider" ... but it is pretty long.

freak4pc · 2017-08-28T15:11:33Z

What about DisposeBagProvider (e.g. it provides a dispose bag)

also @ashfurrow RxSwift itself still uses ObservableConvertibleType etc so I think that's fine.
Problem is "DisposeBagType" means the object itself is a dispose bag, and not that it is able to dispose thing.

Interesting about Krzysztof's article. We can keep it this way for now. I'm still a bit iffy about this :) But that's personal taste.

I'd prefer Disposing (e.g. it can Dispose) or the aforementioned DisposeBagProvider. But let's leave it at HasDisposeBag and be done with it I guess !

twittemb · 2017-08-28T15:21:01Z

ok. Thanks for your feedback anyway.
Just for the record, NSObject-Rx was useful for the project I am doing on my Github. It is about implementing Navigation in iOS apps using a Weaving Pattern. I have experienced many architecture solutions and the best I've found so far was MVVM (with RxSwift) + Coordinator pattern. But I found the Coordinator pattern a bit verbose and there was a lot of boilerplate code to write each time (many delegate patterns). So I combined Coordinator + RxSwift + the idea of navigation state (a bit like Redux) to make a Framework that could ease the implementation of such a pattern. The idea is really to:

remove navigation code from ViewController for a best separation of concerns,
promote dependency injection
allow to cut the storyboards into atomic pieces to ease the reusability (in different navigation contexts) and the collaboration (no more storyboards conflicts).
There is still a lot to do but I have something working. Here is the repo:
https://github.com/twittemb/Weavy

Feel free to contribute :-)

twittemb · 2017-08-30T21:21:02Z

Hi,
What is the validation process for this PR ? Is there a delay to allow the community to review it ?
thanks.

freak4pc · 2017-08-30T21:21:58Z

I think there's agreement with me and @ashfurrow so that's enough.

Let's just get the CI working and LGTM!

ashfurrow · 2017-08-30T21:24:24Z

Yeah, we should definitely get this merged. Any ideas what's wrong with the tests on Travis? I can help take a look tomorrow, let me know.

freak4pc · 2017-08-30T21:25:03Z

I'm on an earlier time zone, I'll take a look around tomorrow my morning :)

toshi0383 · 2017-08-30T22:49:31Z

 EXTERNAL SOURCES:
  NSObject+Rx:
-    :path: "../"
+    :path: ../


I think this needs to be ../NSObject_Rx to make CI happy.

No, this points to the podspec so it should be fine

freak4pc · 2017-08-31T12:27:47Z

Hey @twittemb
Can you remove the Podfile.lock and Cartfile.resolved changes from this commit? I don't think it belongs as part of this change and is probably what causing the CI to fail.

Once that is changed and merged @ashfurrow can separately release a new version

twittemb · 2017-08-31T12:57:54Z

Hey @freak4pc
I have made the changes.
IMHO the Cartfile.resolved should be in the repo. This is what allows to do a "carthage bootstrap" instead of a "carthage update" to ensure the consistency of the dependencies versions.

freak4pc · 2017-08-31T12:59:08Z

@twittemb I didn't mean to remove the entire Podfile.lock ... I mean you should not have changed it... Can you fix that ? Right now you just deleted the entire file. Regarding Carthage.resolved, might be but that shouldn't be part of this specific PR IMO

twittemb · 2017-08-31T13:03:41Z

@freak4pc ok sorry for the misunderstanding ... I've restored the Podfile.lock with the changes you recommend.

freak4pc · 2017-08-31T13:04:29Z

This PR still introduces Podfile.lock changes...
My point is this PR shouldn't have any changes on Podfile.lock or Cartfile.resolved at the moment.

In this commit, we try to conform to protocol oriented programming. It is more flexible to introduce a HasDisposeBag protocol so that every object that respect this one can have a DisposeBag (NSObject does respect that protocol now)

twittemb · 2017-08-31T13:10:05Z

@freak4pc should be alright now :-)

freak4pc · 2017-08-31T13:10:49Z

@twittemb Yup! looks good! Let's see if CI clears and I'm good to merge this.
Thanks for your patience and cooperation !

freak4pc · 2017-08-31T14:22:07Z

Very confusing 🤔
Master is passing and there's only code changes, so not sure why the module isn't passing.

@ashfurrow any thoughts here? I didn't setup the CI here so not sure if any assumptions I'm missing

ashfurrow · 2017-08-31T15:07:15Z

It's possible that Travis has changed something on their end. I'll investigate and get back to everyone.

twittemb · 2017-08-31T15:09:15Z

ok thanks @ashfurrow
If this can help, I can still make a first PR that would remove Demo app and integrate the tests that were done in it directly into the main NSObject-Rx project. I could rebase the current PR on this new one.

freak4pc · 2017-08-31T15:10:03Z

@twittemb If you have the time that would be great! Might also resolve our issue here altogether.

twittemb · 2017-08-31T15:16:22Z

@freak4pc @ashfurrow I'll try to do it today (Montreal time zone)

ashfurrow · 2017-08-31T15:20:18Z

I can reproduce the problem locally – will have a fix shortly. We might as well keep this PR then.

ashfurrow · 2017-08-31T15:48:57Z

Okay, so this looks like an instance of a pretty common issue – Carthage and CocoaPods not being in sync. Happens all the time on Moya :) I've opened #42 with the changes from this PR as well as the changes needed to sort things out.

…Bagable Merges #41

ashfurrow · 2017-08-31T16:28:52Z

Thanks a lot for contributing! I've invited you to join the RxSwiftCommunity GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions.

ashfurrow approved these changes Aug 28, 2017

View reviewed changes

toshi0383 reviewed Aug 30, 2017

View reviewed changes

disposeBag: add a HasDisposeBag protocol

99868e3

In this commit, we try to conform to protocol oriented programming. It is more flexible to introduce a HasDisposeBag protocol so that every object that respect this one can have a DisposeBag (NSObject does respect that protocol now)

freak4pc changed the title ~~disposeBag: add a DisposeBagable protocol~~ disposeBag: add a HasDisposeBag protocol Aug 31, 2017

ashfurrow mentioned this pull request Aug 31, 2017

Merges #41 #42

Merged

ashfurrow merged commit 99868e3 into RxSwiftCommunity:master Aug 31, 2017

ashfurrow added a commit that referenced this pull request Aug 31, 2017

Merge pull request #42 from RxSwiftCommunity/twittemb-feature/dispose…

2ffa937

…Bagable Merges #41

twittemb deleted the feature/disposeBagable branch August 31, 2017 17:12

ashfurrow mentioned this pull request Oct 7, 2017

rx.disposeBag may be using wrong address #47

Closed


		It'll work just like a property: when the instance is deinit'd, the `DisposeBag` gets disposed. It's also a read/write property, so you can use your own, too.

		If you want to add a DisposeBag to an Object that does not inherit from NSObject, you can also implement the protocol "DisposeBagable", and you're good to go. This protocol provides a default DisposeBag called rxDisposeBag.

Conversation

twittemb commented Aug 28, 2017

Uh oh!

ashfurrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freak4pc commented Aug 28, 2017

Uh oh!

twittemb commented Aug 28, 2017

Uh oh!

freak4pc commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freak4pc commented Aug 28, 2017

Uh oh!

twittemb commented Aug 28, 2017

Uh oh!

twittemb commented Aug 28, 2017

Uh oh!

freak4pc commented Aug 28, 2017

Uh oh!

ashfurrow commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freak4pc commented Aug 28, 2017

Uh oh!

twittemb commented Aug 28, 2017

Uh oh!

freak4pc commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashfurrow commented Aug 28, 2017

Uh oh!

twittemb commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freak4pc commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twittemb commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twittemb commented Aug 30, 2017

Uh oh!

freak4pc commented Aug 30, 2017

Uh oh!

ashfurrow commented Aug 30, 2017

Uh oh!

freak4pc commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freak4pc commented Aug 31, 2017

Uh oh!

twittemb commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freak4pc commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twittemb commented Aug 31, 2017

Uh oh!

freak4pc commented Aug 28, 2017 •

edited

Loading

ashfurrow commented Aug 28, 2017 •

edited

Loading

freak4pc commented Aug 28, 2017 •

edited

Loading

twittemb commented Aug 28, 2017 •

edited

Loading

freak4pc commented Aug 28, 2017 •

edited

Loading

twittemb commented Aug 28, 2017 •

edited

Loading

twittemb commented Aug 31, 2017 •

edited

Loading

freak4pc commented Aug 31, 2017 •

edited

Loading