Skip to content

1.x: Change the signature of ignoreElements is also an implicit cast to whatever type you want. #3451

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

Closed
wants to merge 1 commit into from

Conversation

abersnaze
Copy link
Contributor

to remove the need for mergeWithEmpty and concatWithEmpty in #3430. I'm pretty sure that this doesn't count as a breaking change because of type erasure.

public final Observable<T> ignoreElements() {
return lift(OperatorIgnoreElements.<T> instance());
public final <R> Observable<R> ignoreElements() {
return lift(OperatorIgnoreElements.<R, T>instance());
Copy link
Member

Choose a reason for hiding this comment

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

Actually, there is no need to add more type parameters but just do an unchecked cast or raw type cast:

@SuppressWarnings("unchecked")
//...
return (Observable)lift(OperatorIgnoreElements.<T>instance());

Note that instance() does this re-cast as well since the operator is stateless.

@davidmoten
Copy link
Collaborator

ah that's great @abersnaze, so simple!

@@ -92,7 +92,7 @@ public void call(Integer t) {
}
})
//
.ignoreElements()
.<Integer>ignoreElements()
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it prove this breaks the source compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it doesn't prove if it breaks binary compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to check binary compatibility as part of CI, there are some checker tools that we can include into the build process. Probably, it deserves a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I came across a semver for Java library awhile ago that could take two jars and hint if it was major, minor or patch change.

Copy link
Member

Choose a reason for hiding this comment

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

I just realized we don't have a guidance on source compatibility explicitly. We only discussed backward compatibility in #1917. But I'm supposed backward compatibility should include both binary and source backward compatibility.

@davidmoten
Copy link
Collaborator

So we need a decision on the importance of source compatibility specifically in the category of generic types, right?

I'm happy to add a generic type to ignoreElements where necessary to my codebases when I update to the next release because for a Java only codebase static typing will show me where things went wrong at compile time. Does anyone know if this change would bring about runtime grief for one of the dynamic languages that use RxJava?

@akarnokd akarnokd changed the title Change the signature of ignoreElements is also an implicit cast to whatever type you want. 1.x: Change the signature of ignoreElements is also an implicit cast to whatever type you want. Nov 12, 2015
@stealthcode
Copy link

Could this changed be done in a minor release?

@abersnaze
Copy link
Contributor Author

closing in favor of #3567

@abersnaze abersnaze closed this Dec 14, 2015
@abersnaze abersnaze deleted the ignore-cast branch January 28, 2016 22:31
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.

6 participants