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

Examine the possibility to replace Stream<X> with BaseStream<X, ?> in method parameters #76

Open
amaembo opened this issue Mar 9, 2016 · 5 comments

Comments

@amaembo
Copy link
Owner

amaembo commented Mar 9, 2016

It's possible to change some StreamEx method signatures to avoid explicit boxing call for parameter:

append(Stream<? extends T>) -> append(BaseStream<? extends T, ?>)
prepend(Stream<? extends T>) -> prepend(BaseStream<? extends T, ?>)

[Implemented] zipWith(Stream<V>[, BiFunction]) -> zipWith(BaseStream<V, ?>[, BiFunction])

headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends Stream<R>>) ->
    headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends BaseStream<R, ?>>)
headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends Stream<R>> mapper,
        Supplier<? extends Stream<R>> supplier) ->
    headTail(BiFunction<? super T, ? super StreamEx<T>, ? extends BaseStream<R, ?>> mapper,
            Supplier<? extends BaseStream<R, ?>> supplier)

Also MoreCollectors method:

flatMapping(Function<? super T, ? extends Stream<? extends U>> mapper
        [, Collector<? super U, A, R> downstream]) ->
    flatMapping(Function<? super T, ? extends BaseStream<? extends U, ?>> mapper
            [, Collector<? super U, A, R> downstream])

This change will allow to skip .boxed() call in some cases. E.g.:

StreamEx.of("a", "b", "c").zipWith(IntStreamEx.ints())...

This change breaks binary compatibility (recompilation will be necessary), so cannot be introduced in patch release.

@amaembo amaembo changed the title Examine the possibility to replace Stream<X> with BaseStream<X, ?> in method parameters Examine the possibility to replace Stream<X> with BaseStream<X, ?> in method parameters Mar 9, 2016
@lukaseder
Copy link

This change breaks binary compatibility (recompilation will be necessary), so cannot be introduced in minor release.

Hmm, interesting... Personally, I think that breaking binary compatibility between minor releases is OK. After all, I don't see why anyone would just patch a library, incrementing minor releases in a productive build without re-running all build/integration testing, etc.

In this case, though: Why not just overload the methods? Chances are, you're going to eventually overload them with variants that accept Iterator<? super T> and/or Iterable<? super T>, and even T...

@amaembo
Copy link
Owner Author

amaembo commented Mar 9, 2016

As for append and prepend: they are already overloaded with T... and Collection<T>. The headTail and flatMapping methods could not be overloaded due to the same erasure (actually for these methods binary compatibility is not violated). Overloading with Iterable<T> is bad: this may break source compatibility as StreamEx extends Iterable, so ambiguity will be introduced. Having overloads like append(Stream) and append(BaseStream) looks redundant and causes the bloat. Here I would better sacrifice the binary compatibility than to add an overload.

I still think that the last version digit (patch) updates should guarantee binary/source compatibility to some extent. Though according to semantic versioning I can do whatever I want before version 1.0.0. Here I just mean that this change should be introduced in 0.6.0, not in 0.5.6 (there will be no 0.5.6 actually).

@lukaseder
Copy link

Overloading with Iterable is bad: this may break source compatibility as StreamEx extends Iterable, so ambiguity will be introduced.

Yes, I've noticed the same thing in jOOλ. Solution: Overload it 3 times: x(Stream), x(Seq), x(Iterable). This way, there will always be a most-specific type for the compiler to pick.

I still think that the last version digit (patch) updates should guarantee binary/source compatibility to some extent

I completely agree. Although, you said "minor" before editing. Patch releases should remain binary compatible at all costs.

@amaembo
Copy link
Owner Author

amaembo commented Mar 9, 2016

This still makes problems for those folks who tries to live in both worlds (I don't know for sure, but probably they exist!): Seq.of(1,2,3).append(StreamEx.of(4,5,6)) fails to compile but StreamEx.of(4,5,6).append(Seq.of(1,2,3)) works currently.

Although, you said "minor" before editing

My bad.

@lukaseder
Copy link

This still makes problems for those folks who tries to live in both worlds

Well ;-) Just kidding. I haven't thought of this, and you're right. I suspect the source of this confusion is the fact that Stream is not Iterable. I wish it were, but apparently there is some forward compatibility issue with Valhalla.

amaembo added a commit that referenced this issue Nov 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants