1.x: Observable.ignoreElementsThen

[davidmoten]

As discussed in #3113.

I also considered on a consistency basis calling it ignoreElementsWith but I don't think it carries the same clarity of meaning like startWith or concatWith. "ignore" and "then" seems to be a more natural combination in English to capture the sequential nature.

[akarnokd]

I'd also add a test to see if the source ignored emits an error and the other is not even subscribed. I'd do this in case one decides to write an operator directly for this use case instead of using concat.

[akarnokd]

Looks good but it may be confusing to users when they encounter this and concatEmptyWith() or mergeEmptyWith(), or even a backport of the Completable class.

[davidmoten]

Looks good but it may be confusing to users when they encounter this and concatEmptyWith() or mergeEmptyWith(), or even a backport of the Completable class.

Those operators are a little bit different and even though ignoreElementsThen can be expressed as .ignoreElements().concatEmptyWith() I think we get enough questions from user land about this issue to warrant first class support (an explicit method).

In terms of using Completable I'm assuming that ignoreElements could be modified to return a Completable and that Completable would have a concatWith operator that did the same as concatEmptyWith. That would be nice though it might be a problem for API backwards compatibility in v1.

[davidmoten]

Extra test has been added, thanks @akarnokd

[stealthcode]

This operates very similarly to #3430 concatEmptyWith. To my knowledge the functional difference between the two is that ignoreElementsThen will drop all onNexted data while concatEmptyWith will blow up if onNext is called. It seems to me that we have the following choices:

1. Merge both functionality but rename one to better align the API (i.e. rename ignoreElementsThen to concatEmptyWithUnchecked and create a mergeEmptyWithUnchecked).
2. Merge this pull request and get the unchecked functionality (auto-drop onNexted values).
3. Merge New operators: concatEmptyWith and mergeEmptyWith #3430 and get the checked functionality (trigger an onError when onNext is called).

@abersnaze @akarnokd preferences? I vote for option 3 above.

[davidmoten]

I think one thing that should drive this decision is how often the functionality of ignoreElementsThen is requested on question and answer sites. It is such a frequent use case that I think it deserves first class support.

[akarnokd]

Tough. I don't have any use case that would require throwing an error if the first source happens to be non-empty and even if were so, I'd use doOnNext( crash ). I'd go for option 2 and seriously think about a shorter method name.

[stealthcode]

How about andThen?

[akarnokd]

Sounds good to me.

[davidmoten]

Unfortunately andThen doesn't carry the meaning of ignoring the emissions of the source and could be confused with concatWith.

[abersnaze]

I vote for number 3 with the option to drop/onError when onNext is called.

[davidmoten]

I prefer #1 without a rename of ignoreElementsThen (haven't seen a better name yet)

[stealthcode]

So far we have 3 totally different proposals and no consensus.

1. Proposed by @akarnokd - Merge this pull request with a shorter name.
2. Proposed by @abersnaze - Change the operator in New operators: concatEmptyWith and mergeEmptyWith #3430 to have an overload to drop the values or onError. If we pick option 2 then would we merge this pull request? Would the functionality would be subsumed?
3. Proposed by @davidmoten - Rename the operator in New operators: concatEmptyWith and mergeEmptyWith #3430 to something that better aligns with ignoreElementsThen and merging them both.

Option 2 above gives us both behaviors with a unified name. I vote for a variation of 2...

1. Rename ignoreElementsThen to andThen. Also the semantics of the concat used internally should eagerly subscribe (to prevent issues with refcounted subscribers).

public Observable<R> andThen(Observable<R> doNext);
public Observable<R> andThen(Values doForValues, Observable<R> doNext);
public enum Values { Ignore, Error }

example callsite...

return inputs.publish(is -> 
    doSomeWork(Values.Ignore, is).andThen(is).retryWhen(this::tryRetry)
);

Thoughts? I'd like to close the books on these 2 PRs.

[stealthcode]

Or instead of an overload I'd be okay with adding a .none() that enforced no onNext values.

[davidmoten]

As mentioned above andThen suffers from this:

Unfortunately andThen doesn't carry the meaning of ignoring the emissions of the source and could be confused with concatWith.

[stealthcode]

The method name sounds fine to me. I'm thinking about the API of a
Completable which makes sense that it's logically one step of completable
work and then the next.

On Fri, Dec 4, 2015, 20:05 Dave Moten [email protected] wrote:

As mentioned above andThen suffers from this:

Unfortunately andThen doesn't carry the meaning of ignoring the emissions
of the source and could be confused with concatWith.

—
Reply to this email directly or view it on GitHub
#3443 (comment).

[davidmoten]

I'm sounding a bit repetitive but nothing about 'andThen' suggests that it
suppresses the onNext emissions of the first Observable. That's an issue
for readability and discoverability.

On Sun, 6 Dec 2015 09:48 Aaron Tull [email protected] wrote:

The method name sounds fine to me. I'm thinking about the API of a
Completable which makes sense that it's logically one step of completable
work and then the next.

On Fri, Dec 4, 2015, 20:05 Dave Moten [email protected] wrote:

As mentioned above andThen suffers from this:

Unfortunately andThen doesn't carry the meaning of ignoring the emissions
of the source and could be confused with concatWith.

—
Reply to this email directly or view it on GitHub
#3443 (comment).

—
Reply to this email directly or view it on GitHub
#3443 (comment).

[stealthcode]

@davidmoten I'm really not concerned with the name that much. I think that it's fairly clear.

What I'd really like to see happen is this...

1. Merge the Completable PR 1.x: Completable class to support valueless event composition + tests #3444.
2. Add an instance method to Observable public Completable none() that ignores all values onNexted and returns a Completable.
3. Add an instance method in Completable public Observable<T> concat(Observable<T>)

The usage of this would look like this.

public Observable<T> writeData(Observable<T> inputs) {
    return inputs.publish(i -> {}
        Completable work = database.insert(i).doOnNext(this::logQuery).none();
        return results.concat(i);
    });
}

This reduces a.ignoreElements().castAs(R.class).concatWith(b) to a.none().concat(b) and it's composable. I believe that @akarnokd recommended a similar approach in #3430

Now that #3444 is merged we could add these 2 methods.

[davidmoten]

That sounds ok

On Tue, 8 Dec 2015 10:05 Aaron Tull [email protected] wrote:

@davidmoten https://github.com/davidmoten I'm really not concerned with
the name that much. I think that it's fairly clear.

What I'd really like to see happen is this...

1. Merge the Completable PR 1.x: Completable class to support valueless event composition + tests #3444
   1.x: Completable class to support valueless event composition + tests #3444.
2. Add a method Observable.none() that ignores all values onNexted and
   returns a Completable.
3. Add an instance method in Completable public Observable
   concat(Observable)

The usage of this would look like this.

public Observable writeData(Observable inputs) {
return inputs.publish(i -> {}
Completable work = database.insert(i).doOnNext(this::logQuery).none();
return results.concat(i);
});
}

This reduces a.ignoreElements().castAs(R.class).concatWith(b) to
a.none().concat(b) and it's composable. I believe that @akarnokd
https://github.com/akarnokd recommended a similar approach in #3430
#3430

Now that #3444 #3444 is merged
we could add these 2 methods.

—
Reply to this email directly or view it on GitHub
#3443 (comment).

[stealthcode]

Great. I like this better as well. @abersnaze @akarnokd - do you guys agree with this approach?

[abersnaze]

I don't particularly like none() maybe toCompletable() to match toSingle() otherwise a solid plan.

[stealthcode]

Next step would be to implement Completable#merge(Observable<T>)... but I'm not convinced on naming just a thought.

[stealthcode]

I opened a PR adding Observable<T> andThen(Observable<T>) to Completable #3570. Please comment. I'm open to different method name options.

[stealthcode]

@NiteshKant is this an acceptable resolution to your PR #3430? Can that PR be closed?

[stealthcode]

Since Completable now has an option to append an Observable I think this PR can be closed. Please comment if it needs to be reopened.