Skip to content
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

Bring back static factories again, this time with autocomplete! #88

Merged
merged 18 commits into from
Sep 28, 2017

Conversation

ZacSweers
Copy link
Collaborator

@ZacSweers ZacSweers commented Sep 21, 2017

This is yet another attempt at static factories, but I feel pretty good about this.

Prior art: #61 #17 #42 #43

Basically - the static API wasn't possible before because of java generics issues. I could never find a sweet spot of conciseness/readability and tooling support (basically - IDE autocomplete). After playing with this more though, I think I've found the sweet spot.

The IDE will happily autocomplete generics IFF there are no parameters. I don't know why, but yeah that's how it is. Doesn't matter if the parameters are generic themselves either, just presence alone. This kind of works ok though, as we can leverage this as a plus to separate scope capture separately to make a single point of entry, then fan out to types.

The gist of how this works is that there's a single AutoDispose class with three public static overloads of with(), each taking one of the supported scope types. These return an implementation of a ScopeHandler, which is just an interface that has 5 methods (one for each rxjava type, aka observable(), single(), etc). Because these methods don't have parameters and the generics are still on the method signature, the IDE autocompletes! This feels a little more readable too, similar to Picasso's API. One extra key stroke, but more obvious to read than "ObservableScoper" IMO.

It's effectively a sort of rehash of #5, but only scope and type since we can leverage the to() operator now.

If this looks good from an API perspective here, I'll update the PR with tests/docs and deprecate the scoper API (with the intention of removing/making it not public it by 1.0). This should be pretty easy to migrate via structural replace as well

Now code looks like this:

Flowable.just(3)
    .to(AutoDispose.with(this).<Integer>flowable())
    .subscribe();

Observable.just(3)
    .to(AutoDispose.with(this).<Integer>observable())
    .subscribe();

Single.just(3)
    .to(AutoDispose.with(this).<Integer>single())
    .subscribe();

Maybe.just(3)
    .to(AutoDispose.with(this).<Integer>maybe())
    .subscribe();

Completable.complete()
    .to(AutoDispose.with(this).completable())
    .subscribe();

And an added bonus - generic isn't necessary in Java 8!

Observable.just(1)
    .to(AutoDispose.with(this).observable())
    .subscribe(integer -> {
      // Yay it's an integer!
    });

@ZacSweers
Copy link
Collaborator Author

CC @uber/mobile-platform-android

@AttwellBrian
Copy link

Seems a little nicer. I like starting everything with AutoDispose so that all you need to know is the name AutoDispose before autocomplete kicks in. And I like the elimination of an empty diamond in java 8.

To be fair:

  • Regarding increasing readability over the non-static factory: We could have always called the scopers things like AutoDipose{Observable, Maybe, Single}Scoper
  • Regarding generics not being necessary in java8: You can already use the empty diamonds like .to(new ObservableScoper<>(this)). Its not like you have to fill in the generics.

Definitely isn't worse than before. Do you feel strongly enough to make a breaking change?

@ZacSweers
Copy link
Collaborator Author

We could have always called the scopers things like AutoDipose{Observable, Maybe, Single}Scoper

True, but that's even wordier. My main argument against Scoper is that it's not really obvious what Scoper is for. It's a thing that scopes, but it's less obvious than AutoDispose with ____ for ____. Which makes me wonder, maybe it should be forObservable() on the methods rather than just the type.

True on generics, but less is more :)

We're not at 1.0 yet, so it's ok. I'd deprecate scopers in 0.3.0 and provide some structural replace templates in the changelog, then remove for 1.0.


public final class AutoDispose {

public interface ScopeHandler {
Copy link
Member

Choose a reason for hiding this comment

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

Would love to see some javadocs for these public, consumer-focused methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely. Just wanted to get feedback on the API first before doing a doc and tests pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ZacSweers
Copy link
Collaborator Author

Seeeeeeems like people are ok with this, so going to go through and update the docs and tests to use this new API.

One change I'm going to poll to some contacts and possibly make - instead of saying .observable(), do .forObservable(). Feel free to chime in here with thumbs up/down reacts on this if you have an opinion

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Sep 23, 2017

Notes for structural replace. If using Java 8, replace <$Type$> with <> and remove <$Type> from the replacement template. Would be ideal to combine these into one smart replace but the tooling is a little rough to debug. Will look more before release to include in the changelog.

Flowable

Java 8 / Object streams

Search template

$Stream$.to(new com.uber.autodispose.FlowableScoper<>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).forFlowable())

Java 7 regular types

Search template

$Stream$.to(new com.uber.autodispose.FlowableScoper<$Type$>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).<$Type$>forFlowable())

Observable

Java 8 / Object streams

Search template

$Stream$.to(new com.uber.autodispose.ObservableScoper<>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).forObservable())

Java 7 regular types

Search template

$Stream$.to(new com.uber.autodispose.ObservableScoper<$Type$>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).<$Type$>forObservable())

Maybe

Java 8 / Object streams

Search template

$Stream$.to(new com.uber.autodispose.MaybeScoper<>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).forMaybe())

Java 7 regular types

Search template

$Stream$.to(new com.uber.autodispose.MaybeScoper<$Type$>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).<$Type$>forMaybe())

Single

Java 8 / Object streams

Search template

$Stream$.to(new com.uber.autodispose.SingleScoper<>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).forSingle())

Java 7 regular types

Search template

$Stream$.to(new com.uber.autodispose.SingleScoper<$Type$>($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).<$Type$>forSingle())

Completable

Search template

$Stream$.to(new com.uber.autodispose.CompletableScoper($Scope$))

Replacement template

$Stream$.to(com.uber.autodispose.AutoDispose.with($Scope$).forCompletable())

@ZacSweers
Copy link
Collaborator Author

Tests and docs updated now, along with a rebase. Seeking opinions on the final namings but this is review-ready from an implementation standpoint @tyvsmith @jbarr21 @AttwellBrian

@ZacSweers ZacSweers merged commit 040bbb0 into master Sep 28, 2017
@ZacSweers ZacSweers deleted the z/staticFactoriesAgain branch September 28, 2017 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants