Skip to content

Android-centric package reorganization (#65) - #66

Merged
JakeWharton merged 4 commits into
ReactiveX:0.xfrom
omo:reorg
Nov 21, 2014
Merged

Android-centric package reorganization (#65)#66
JakeWharton merged 4 commits into
ReactiveX:0.xfrom
omo:reorg

Conversation

@omo

@omo omo commented Nov 21, 2014

Copy link
Copy Markdown
Contributor

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

dpsm commented Nov 21, 2014

Copy link
Copy Markdown
Contributor

Looks like we are on the right track! 👍

Copy link
Copy Markdown
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
Copy Markdown
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

omo commented Nov 21, 2014

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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