Skip to content

Conversation

quinmars
Copy link
Contributor

@quinmars quinmars commented Jun 3, 2018

This PR will add the Rx counter parts to the IEnumerable operators Append and Prepend, see #420. It's still work in progress, because there are some open questions.

  1. As said in Add Prepend and Append to Rx #420 I prefer to mirror the API of the IEnumerable operators exactly, i.e., not the variadic form. But if you favor the variadic form I can also implement that one.

  2. Currently, I've implemented it with source.Concat(Observable.Return(value, scheduler)), but the results are slightly different to source.Concat(new[] { value }.ToObservable(scheduler)). Using the test scheduler the OnComplete will come at the same tick like the value (see the unit test). In the second case it comes one tick after the last OnNext. I don't have a deep knowlege of the schedulers and which variant is more reasonable. The second variant is very similar to StartWith and how a n-ary append, would be implemented.

Right now there is only the code for the append operator. Once the two questions are answered I'll add the prepend operator.

@danielcweber
Copy link
Collaborator

danielcweber commented Jun 4, 2018

Thanks.

I will happily merge all these new operators once I get a direction on those kind of changes from @onovotny or @ghuntley. Or maybe there is already a roadmap and I missed it??

@quinmars This is perfectly fine to get the operators into Rx. Do you think you could, at some point, provide optimized versions of them that implement the logic directly instead of combining Concat and Return?

@quinmars
Copy link
Contributor Author

quinmars commented Jun 4, 2018

I definately thought about adding an improved version. We will see how far I will get. Maybe I'll need your help at some point. With that in mind, it would probably be better to use source.Concat(new[] { value }.ToObservable(scheduler)) as reference implementation. Than the improved version could also be used for StartsWith without a change in its behavior.

@danielcweber
Copy link
Collaborator

Sure, just give me a hint if you need help!

@akarnokd
Copy link
Collaborator

akarnokd commented Jun 5, 2018

This one is easy, just create a plain forwarding operator and in its OnCompleted, simply call ForwardOnNext(appendedValue); ForwardOnCompleted().

@quinmars
Copy link
Contributor Author

quinmars commented Jun 7, 2018

@akarnokd Thanks, indeed, appending and probably also prepending doesn't look to be terribley complicated.

@danielcweber
Copy link
Collaborator

If you had fun implementing Prepend, you could take a look at StartWith, which is currently also implemented like this:

return values.ToObservable(scheduler).Concat(source);

There's definitely room for improvement.

@quinmars
Copy link
Contributor Author

quinmars commented Jun 7, 2018

I now added the corresponding Prepend() operator. I used for now the StartWith() operator as reference implementation, because my initial goal was to mimic the behaivor of StartWith() as close as possible. Than you can simply advice to use Prepend instead of StartWith when you only want to add one element. But the behavior of StartWith is at least a little bit suprinsingly, because it does not only schedule the value to be appended, but also OnCompleted. That's also different to how Return + Concat works.

Would do you think is the "right" or best behavior of Prepend:

  1. Being close to StartWith and ToObservable + Concat
  2. Being close to Return + Concat

@quinmars
Copy link
Contributor Author

quinmars commented Jun 7, 2018

@danielcweber Yes, I'll take a look on it once Append and Prepend are optimized. But I fear that one is more difficult. :)

@danielcweber
Copy link
Collaborator

Where is the combination of Return + Concat used currently?

@quinmars
Copy link
Contributor Author

quinmars commented Jun 7, 2018

@danielcweber No where. Except in Append but that doesn't really count.

@danielcweber
Copy link
Collaborator

Then you should go with the StartWith-behaviour.

Also, why shouldn't Prepend become a special overload of StartWith? Where does the name Prepend come from?

@quinmars
Copy link
Contributor Author

quinmars commented Jun 7, 2018

See #420, Enumerable.Append and Enumerable.Prepend are now part of CoreFx and maybe of netstandard2 (I haven't checked that though).

@akarnokd
Copy link
Collaborator

akarnokd commented Jun 7, 2018

I'd be careful with this. Concat uses the quite complicated TailRecursiveSink with all sorts of unrolling and combining with other Concat operators, basically flattening the stack when you chain Concat after Concat.

@quinmars
Copy link
Contributor Author

quinmars commented Jun 7, 2018

@akarnokd Carefull with what? Optimizing StartWith?

@akarnokd
Copy link
Collaborator

akarnokd commented Jun 7, 2018

Yes.

@quinmars
Copy link
Contributor Author

quinmars commented Jun 7, 2018

@akarnokd That's the last point on my list. And if it gets to complicated for me, I'll happily leave it up to you both. :)

@quinmars
Copy link
Contributor Author

So, I changed the Append() method to match the Prepend() behavior, i.e., scheduling the OnCompleted() call. Meanwhile I started to write optimized implementations of the two operators. I guess they'll be ready for review in the next days. Should I push them to this PR or would it be better to open a new one? If the latter is the case I would remove the WIP tag.

@quinmars
Copy link
Contributor Author

BTW, Enumerable.Append() and Enumerable.Prepend() are already part of netstandard1.6.

# Conflicts:
#	Rx.NET/Source/src/System.Reactive/Linq/Qbservable.Generated.cs
@danielcweber
Copy link
Collaborator

I could merge it now to have a reference implementation and working tests. You can go either way, if you remove the WIP-tag, I'll merge it.

@quinmars quinmars changed the title [WIP] Add the Append and Prepend operator Add the Append and Prepend operator Jun 14, 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.

3 participants