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

Make cast and ofType operate on wildcard types rather than Any #176

Merged
merged 2 commits into from Jul 17, 2018

Conversation

ansman
Copy link
Contributor

@ansman ansman commented Apr 8, 2018

Maybe.ofType, Maybe.cast and Single.cast are declared as extensions on Maybe<Any>/Single<Any> rather than on Maybe<*>/Single<*> which prevents them from being used in some cases.

This PR fixes this and will align them with how the Observable and Flowable extensions are implemented.

This would be a binary compatible change and all existing consumers wouldn't need to change anything.

@stepango
Copy link
Collaborator

@ansman could you provide few examples of cases you are talking about?

@ansman
Copy link
Contributor Author

ansman commented Apr 10, 2018

The most obvious example that doesn't work is this:

val maybe: Maybe<*> = Maybe.just("foo")
val stringMaybe = maybe.ofType<String>()

You can only cast Maybe to Maybe<*> if you don't want unchecked warnings so it would make sense to support them.

And besides, it should be implemented like it is for Observable and Flowable which it currently isn't.

@stepango
Copy link
Collaborator

@ansman but this example is not correct from RxJava perspective * effectively mean Any? but potentially you can't have nulls in your stream.

val maybe: Maybe<Any> = Maybe.just("foo")
val stringMaybe = maybe.ofType<String>()

will work just fine.

@JakeWharton
Copy link
Member

Sure but then the other extensions are needlessly wide in the types they accept.

I don't see any reason to not accept this PR for parity's sake alone.

@D4Muck
Copy link
Contributor

D4Muck commented Jul 3, 2018

I mean just a simple Single.just("test").cast<CharSequence>() does not work...
Type mismatch: inferred type is Single<String> but Single<Any> was expected

So I would vote for this to get merged

@thomasnield
Copy link
Collaborator

I have a sense that this goes beyond the scope of RxKotlin. Such extension functions can be implemented on one's own domain if they are needed.

@ansman
Copy link
Contributor Author

ansman commented Jul 17, 2018

I'm sorry but this feels like an extremely bad reason for closing this. This does not add, change or remove functionally but rather fix something that is, in my opinion, broken.

The built in cast function works on all types of observables, including Observable<?>. Therefore it stands to reason that these extensions should too.

@thomasnield thomasnield reopened this Jul 17, 2018
@thomasnield
Copy link
Collaborator

Sorry I completely skimmed through this too quickly. This makes sense.

@thomasnield
Copy link
Collaborator

@ansman can you resolve conflict and then I will merge?

@ansman
Copy link
Contributor Author

ansman commented Jul 17, 2018

@thomasnield Thanks, fixed

@thomasnield thomasnield merged commit d8d6a89 into ReactiveX:2.x Jul 17, 2018
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.

None yet

5 participants