Add of_lwt and of_list_lwt to stream

[bn-d]

No description provided.

[raphael-proust]

Hi,
I haven't had lots of time but I have some now, so I'm reviewing your work now.

[raphael-proust]

I understand the purpose of of_lwt, but I don't understand the motivation behind of_list_lwt. More specifically, I don't imagine this pattern appearing too often. Do you have a concrete use case in mind? How common is this pattern in your code? How come you end up with many 'a list Lwt.t?

I'm tempting to

a. Leave of_list_lwt out because it is quite specific and also because it is the combination of flatten and of_lwt: Lwt_stream.(flatten (of_lwt l)). However, I'm interested in knowing what led you to propose this function and whether flatten+of_lwt would be sufficient for you. And I'm more than happy to revisit change my mind based on your answer.

b. Add an of_lwt_seq : 'a Lwt_seq.t -> 'a t which your PR made me realise is missing. I'll do that in a separate PR to avoid derailing the conversation of this PR.

[bn-d]

I think my use case is that I have stream in and stream out, but some calculations in the middle require the data to be aggregated and processed in Lwt. There is val to_list : 'a t -> 'a list Lwt.t, but is no direct way doing the reverse, turning 'a list Lwt.t back to 'a Lwt_stream.t. Actually, I wrote of_list_lwt first and add of_lwt cause I thought it is was nice addition and could be handy in the future.

flatten + of_lwt does solve the problem. If the above use case does not change you mind, I could modify the PR and remove of_list_lwt.

[raphael-proust]

I see, so you have some steps that can be processed item-by-item using map or something like that, and then occasionally you have steps that need all of the data to be present at the same time. This makes sense.

My understanding of to_list : 'a t -> 'a list Lwt.t is that it transforms a data-structure containing multiple elements in another. The first one doesn't necessarily have all the elements ready at the same time so the function must return a promise of a list rather than a list per se. In that sense, of_list is the converse function transforming a data-structure containing multiple elements in another.

In general, although there are multiple exceptions for that, passing a promise to a function is meant as a kind of synchronisation mechanism. Indeed, in most cases, it is expected that you bind onto the promise, get the data out of it, and pass the data around. It is not a hard, unbreakable rule, but it is a general pattern of code that helps keep the code simple and the synchronisation apparent.

In your case, you could use flatten, but you could also bind in places. That is, you can alternate work on streams and work on list promises. This is less handy if you want to return a stream rather than keep using it until you have the final result.

Does that make sense?

---

As for of_lwt I'd suggest a small change of name to fit with Seq/Lwt_seq:

• val return : 'a -> 'a t where return x is a stream that has x on it and then is closed.
• val return_lwt : 'a Lwt.t -> 'a t (same but when the promise becomes resolved)

[raphael-proust]

I've added some tests. Specifically, tests with some pausing (just so the promise is not a trivial, already resolved one) and some failing. I then fixed the exception handling in the return_lwt case. And I amended the documentation to mention the failing case and to tidy things a little.

Can you check the changes?

Additionally, we should add an entry in the CHANGELOG. What name do you want to be referred by in this changelog? You can apply the following patch and add your name and push that as a commit:

diff --git a/CHANGES b/CHANGES
index a0f7921a5..3e7a46c86 100644
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,8 @@

   * Lwt_seq: a Seq-like data-structure with Lwt delayed nodes (#836, #842, Zach Shipko).

+  * Lwt_stream.return, Lwt_stream.return_lwt: singleton stream constructors (#864, )
+
 ====== Misc ======

   * Code quality improvement: remove an uneeded Obj.magic (#844, Benoit Montagu).

[bn-d]

The change looks good. I also pushed a change for the CHANGELOG. Thanks