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

Extend lwt_seq #842

Merged
merged 16 commits into from
Jul 24, 2021
Merged

Extend lwt_seq #842

merged 16 commits into from
Jul 24, 2021

Conversation

raphael-proust
Copy link
Collaborator

Adds iter_p, renames iter into iter_s, adds iter.

This is meant as a discussion, it should ideally happen before the first release of Lwt that includes Lwt_seq to avoid difficulties with the backwards compatibility.

Are the added functions useful?
Should we also have a similar range of /_sfor some other traversals? (Note that _p doesn't make sense for some of them.)

ping @zshipko @c-cube

@hcarty
Copy link
Contributor

hcarty commented Mar 2, 2021

Seems like a good change to be consistent with the other modules in Lwt. iter_n may be useful as well, like Lwt_stream.iter_n.

@zshipko
Copy link
Contributor

zshipko commented Mar 2, 2021

Agreed, I think these are good changes. iter_n also sounds nice!

@raphael-proust
Copy link
Collaborator Author

I'm adding iter_n. Also map_s (map_p doesn't make as much sense because it breaks laziness.) and filter_s and such.

Other questions:

  • Do we need to wrap some applications of seq () with Lwt.wrap so that exceptions are propagated uniformally whether they are raised when evaluating the first suspension or the subsequent ones? (I think we should do this, I'll make a commit towards this, I'm interested in opinions and comments though.)
  • Should flat_map actually be its current type (val flat_map : ('a -> 'b t Lwt.t) -> 'a t -> 'b t) or should we drop the Lwt.t in the signature? The 'b t is an alias for unit -> 'b node Lwt.t so additional Lwt.t might not be needed. (I think we should drop it, but maybe we could offer flat_map and flat_map_s?)

@hcarty
Copy link
Contributor

hcarty commented Mar 3, 2021

I agree that using Lwt.wrap seems like a good addition.

Dropping the extra Lwt.t from flat_map also seems like a good idea. I'm not sure if flat_map_s is necessary, but keeping flat_map Lwt.t-less is a reasonable match to the rest of Lwt.

@raphael-proust
Copy link
Collaborator Author

* Should `flat_map` actually …

I noticed something similar for of_seq_lwt: it returned a promise ('a t Lwt.t) which doubles the Lwt.tness of it. I've made a patch to fix that. I also fixed some exceptions in the same patch.

Also important, of_seq_lwt and of_seq are now truly lazy: they only start evaluating their argument's first suspended node when the result is actually traversed rather than immediately.

Along the same line, I added return_lwt : 'a Lwt.t -> 'a t and cons_lwt : 'a Lwt.t -> 'a t -> 'a t to be able to construct sequences directly from promises when that's what we have on hand. (return_lwt p is equivalent to fun () -> p >>= return). And also unfold_lwt too.

I've also added some tests for iter_p and iter_n to check that the semantic is correct. I added that via a pseudo-fuzzing (with a fixed small set of functions, underlying sequences, and initial values for the state held in a reference) which compares the result to List.iter.

I think it's mostly ready. It needs documentation and also a pass of cosmestics to make the code more uniform. Feedback welcome before I start doing these. :)

@zshipko
Copy link
Contributor

zshipko commented Mar 13, 2021

Just took a look at the latest changes and everything seems to match the Lwt API better now. Also, the addition of the *_lwt functions makes things much more clear.

@raphael-proust raphael-proust merged commit 92501c2 into ocsigen:master Jul 24, 2021
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