android10 / frodo

Android Library for Logging RxJava Observables and Subscribers.
1.48k stars 101 forks source link

RxJava 2 support #33

Closed VictorAlbertos closed 7 years ago

VictorAlbertos commented 8 years ago

HI @android10

Are you planning to add support for RxJava2?

android10 commented 8 years ago

@VictorAlbertos not until we have final version. And of course PRs are always welcome :).

VictorAlbertos commented 8 years ago

I can give a try ;)

For my own libraries I added support for RxJava2 creating another branch. Just like RxAndroid did it. Does it would be ok for you?

android10 commented 8 years ago

@VictorAlbertos feel free. Honestly sometimes the final api can change so that is why I like to wait until we have a final version. I guess RxJava 2 is mature enough but it does not hurt to wait a bit longer. :). Thanks for the feedback! 👍

VictorAlbertos commented 8 years ago

I'm already working on the PR, but please I want you to take a look at this issue from RxJava:

https://github.com/ReactiveX/RxJava/issues/4515

It seems that Subscriber has been reserved only for Flowable, which is like an Observable but allows to handle backpressure with different strategies (Observable does not handle back pressure anymore). Now, in order to subscribe to an Observable you need to use an Observer.

So, my question is how do you want to address this situation?

I suggest to refactor RxLogSubscriber and its companions to RxLogObserver, that way Frodo will keep supporting Observables. And later if you want we can add support for this new type Flowable.

VictorAlbertos commented 8 years ago

@android10 I finished :)

Take a look at this branch from my fork: https://github.com/VictorAlbertos/frodo/commits/2.x

I'm going to need you to create a branch ("2.x" if you will) in order to perform the PR. Because it's not possible to create a PR to a non-existing branch xD

Some notes:

I hope you find the PR acceptable :)

VictorAlbertos commented 8 years ago

By the way, all test are passing and as far as I can tell the sample module outputs the right messages in the log.

VictorAlbertos commented 7 years ago

Hi @android10

Are you planning to review the changes any time sooner? I said so because I need to start using your library with 2.x version.

Thanks :)

android10 commented 7 years ago

@VictorAlbertos I will review the changes but keep in mind that we need to find a way to make it backward compatible. Replacing everything with the new stuff is not gonna work since I do not want to maintain 2 libraries for 2 different versions of RxJava.

android10 commented 7 years ago

@VictorAlbertos other thing. I'm not gonna ship a version of frodo supporting RxJava 2 until we have a final version.

VictorAlbertos commented 7 years ago

If you don't want to create another branch/module to support 2.x, I think Frodo needs to be refactored in a really heavy way in order to be enterely decoupled from RxJava. And once that refactoring is done, two others modules need to be created, one for 1.x and another one for 2.x, trying to share as much code as possible from that "core" module. But I don't see how this could be achieved with Frodo.

Along the process of porting my own libraries, only Mockery has the kind of structure suitable for this approach. The rest of them required to be another entirely library, hosted in another branch or module.

Nevertheless, if you think I can help you out with this, just let me know :)

android10 commented 7 years ago

@VictorAlbertos yup, definitely needs a refactor. What you suggested is an option but at the moment I will freeze it until we have a final version of RxJava 2. Other point to not work on that yet, is that the adoption is pretty low.

Of course, any idea is always welcome. Again, having 2 separated frodo versions for 2 separated RxJava versions is a lot of overhead, specially with maintenance. Let's get back to this issue soon.

VictorAlbertos commented 7 years ago

Oki then ;)

petrnohejl commented 7 years ago

Hello. Final version of RxJava 2 has been already published. @android10 Are you going to add a support for RxJava 2?

android10 commented 7 years ago

@petrnohejl that is the idea...but still thinking how the approach will be. Any feedback is welcome.

Maybe it is better to create another library that supports only RxJava 2 rather than modifying frodo to support the latest one, since they use different types and packages...

A couple of questions need an answer:

1 - Create frodo2? 2 - In case of supporting both versions of RxJava, what the annotations will look like?

I will appreciate anyone shading some light here.

petrnohejl commented 7 years ago

I think it would make sense to create a completely new library "Frodo 2". RxJava 2 has a different API so supporting both versions of RxJava would require a huge refactoring and changes. Moreover it would have 2 dependencies - both RxJava libs.

VictorAlbertos commented 7 years ago

Yes, it should be another library. Indeed, almost every library out there which is a reactive extension has adopted this approach. Normally, rather than creating a new repo, they create another branch.

About the annotations, I think the repo which I created months ago could be a good starting point.

android10 commented 7 years ago

@VictorAlbertos I have something already working with some refactor, so I will also upload it in order to combina efforts and come up with a new version.

TonyTangAndroid commented 7 years ago

Looking forwarding to Rxjava2 support with "Frodo2"

android10 commented 7 years ago

@TonyTangAndroid It is a WIP. Will come to light soon.

android10 commented 7 years ago

Closing since this is going to happen here: https://github.com/android10/frodo2

AlexTrotsenko commented 6 years ago

I know, that there is this https://github.com/android10/frodo2/issues/1 issue, but is there any plan to publish frodo2 in mavenCentral/jcenter?

I have not looked deep inside, but as far as I see:

VictorAlbertos commented on Sep 9, 2016 By the way, all test are passing and as far as I can tell the sample module outputs the right messages in the log.

android10 commented 6 years ago

@AlexTrotsenko that is the idea. I have not had enough time to work on the project but it is on its way. I will grab it again and generate a preview so people can start using it :)