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

3.x: improve combineLatest method names #6634

Closed
vanniktech opened this issue Aug 27, 2019 · 4 comments
Closed

3.x: improve combineLatest method names #6634

vanniktech opened this issue Aug 27, 2019 · 4 comments

Comments

@vanniktech
Copy link
Collaborator

Calling combineLatest from Kotlin is a bit awkward. The function has a lot of different parameters, including Iterable, Array as well as 2-x call sites for passing multiple Observables.

Is it possible to prefix these so we have some kind of combineLatestFromIterable which takes the Iterable<T>:

-public static <T, R> Observable<R> combineLatest(Iterable<? extends ObservableSource<? extends T>> sources, Function<? super Object[], ? extends R> combiner) {
+public static <T, R> Observable<R> combineLatestFromIterable(Iterable<? extends ObservableSource<? extends T>> sources, Function<? super Object[], ? extends R> combiner) {

and then combineLatest which takes the 2-x call sites.

public static <T1, T2, R> Observable<R> combineLatest(
            ObservableSource<? extends T1> source1, ObservableSource<? extends T2> source2,
            BiFunction<? super T1, ? super T2, ? extends R> combiner) {

The array call we could call combineLatestFromArray.

-public static <T, R> Observable<R> combineLatest(ObservableSource<? extends T>[] sources, Function<? super Object[], ? extends R> combiner) {
+public static <T, R> Observable<R> combineLatestFromArray(ObservableSource<? extends T>[] sources, Function<? super Object[], ? extends R> combiner) {

What should we do with?

public static <T, R> Observable<R> combineLatest(Function<? super Object[], ? extends R> combiner, int bufferSize, ObservableSource<? extends T>... sources) {

Is it really required? Callers could just use the array overload.

combineLatestDelayError is equivalent and could get the method name improvements as well.

Other methods, such as concat/merge could benefit from this change too.

@akarnokd
Copy link
Member

combineLatestArray would be reasonable and match the other xArray method names. What's the conflict between combineLatest(Iterable, F) and combineLatest(O1, O2, F2)?

The varargs variant could be removed.

@vanniktech
Copy link
Collaborator Author

There should not be any issues with combineLatest(Iterable, F) and combineLatest(O1, O2, F2). I'd just go for consistency. Looking at startWithIterable.

@akarnokd
Copy link
Member

startWithIterable was disambiguated due to startWith(Publisher) and/or startWith(T) so I'd say is an exception. So if the main trouble is with combineLatest(O[], F) then let's rename that to combineLatestArray.

zip could use some name clarification too. There is zip(Iterable(P), F), zipIterable(Iterable(P), bool, int, F), zipArray(F, O...), zip(P(P)).

@vanniktech
Copy link
Collaborator Author

Closed by #6635 & #6640

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