Skip to content

Conversation

omo
Copy link
Contributor

@omo omo commented Nov 21, 2014

Here is a first cut.

What seems good for me:

  • Less public classes
  • Less imports

What doesn't look great but not sure how can be better:

  • Breaking change!
  • New facade classes (WigetObservable, ContentObservable) - Feels a bit awkward. Probably better to split into per-counter part classes like TextViewObservable?
  • rx.observable.AndroidSubscriptions - Feels there could be better home for this guy, but I'm not sure where it is :-/

WDYT? Comments are appreciated.

This change introduces new, Android centric packages including

 * rx.android.content for android.content related classes
 * rx.android.view for android.view related classes
 * rx.android.widget for android.widget related classes
 * rx.android.util for package agnostic classes

Then it moves existing classes based on their Android counterparts.

Although one of the goal is to reduce the number of publics,
this change still does nothing but class move. More changes will follow.
@dpsm
Copy link
Contributor

dpsm commented Nov 21, 2014

Looks like we are on the right track! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

revert

@JakeWharton
Copy link
Contributor

LGTM. Couple small things.

omo added 2 commits November 21, 2014 13:05
This allows some contnet-related internal classes package local.
Some ViewObservable methods are for android.widget.* classes.
This change moves  these to newly introduced WidgetObservable.
This turns some internal classes from public to package-local.
@omo
Copy link
Contributor Author

omo commented Nov 21, 2014

Thanks for the view Jake! The updated branch should address the feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in an internal package? I don't think it's our responsibility to expose this in a public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. As there is only one internal class so far, I just renamed util to internal.

@JakeWharton
Copy link
Contributor

This looks great. Will merge on green. Thanks!

JakeWharton added a commit that referenced this pull request Nov 21, 2014
Android-centric package reorganization (#65)
@JakeWharton JakeWharton merged commit 0893101 into ReactiveX:0.x Nov 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants