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.

Sign up for GitHub

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

Add Lwt_result.collect without deps #1040

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+16 −0
Open

Add Lwt_result.collect without deps #1040

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16173d1
add without deps
Nov 15, 2024
f22dc1c
Merge branch 'ocsigen:master' into master
seprov Nov 21, 2024
6c08cd5
Merge branch 'ocsigen:master' into master
seprov Dec 18, 2024
8ec28f1
make more efficient and concise
Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/core/lwt_result.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,29 @@ let iter_error f r =
| Error e -> f e
| Ok _ -> Lwt.return_unit)

let partition_filter_map f l =
let rec iter f l1 l2 l =
match l with
| [] -> (List.rev l1, List.rev l2)
| x :: tl -> (
match f x with
| `Left y -> iter f (y :: l1) l2 tl
| `Right y -> iter f l1 (y :: l2) tl
| `Drop -> iter f l1 l2 tl)
in
iter f [] [] l

let split_result results =
partition_filter_map
(fun x -> match x with Ok o -> `Left o | Error e -> `Right e)
results

let collect x =
x |> Lwt.all
|> Lwt.map (fun r ->
let oks, errors = split_result r in
match errors with [] -> Ok oks | _ -> Error errors)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a quite specific note: This seems like a simple yet costly way to implement collect. E.g., the oks list is reversed (line 110) whether there are errors or not.

On a more general note: the fact that the Lwt part and the result part of this function are entirely independent makes me wonder whether it actually belongs in Lwt.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback.

This seems like a simple yet costly way to implement collect

You're right. If we wanted to add collect to Lwt_result, then I'd be happy to optimize this.

the fact that the Lwt part and the result part of this function are entirely independent makes me wonder whether it actually belongs in Lwt

You're also right that the Lwt and result parts are distinct. If, for example, collect were in CCResult as

let collect x =
  let oks, errors = split_result x in
  match errors with [] -> Ok oks | _ -> Error errors

then usage might look like

my_data_list
|> request_for_each
|> Lwt.all |> Lwt.map CCResult.collect

which isn't much different from or less convenient than

my_data_list
|> request_for_each
|> Lwt_result.collect

If you think that, even with a more efficient implementation of result list filtering and partitioning, this code does not belong in Lwt, then I'd be happy to close this PR.


module Infix = struct
let (>>=) = bind
let (>|=) e f = map f e
Expand Down
8 changes: 8 additions & 0 deletions src/core/lwt_result.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ val iter_error : ('e -> unit Lwt.t) -> ('a, 'e) t -> unit Lwt.t
@since Lwt 5.6.0
*)

val collect : ('a, 'b) result Lwt.t list -> ('a list, 'b list) result Lwt.t
(** [collect r] is a single promise resolved with [Ok u] if all promises
in [r] resolve with [Ok v] and a single promise resolved with [Error w]
otherwise.

@since NEXT_RELEASE
*)

module Infix : sig
val (>|=) : ('a,'e) t -> ('a -> 'b) -> ('b,'e) t
val (>>=) : ('a,'e) t -> ('a -> ('b,'e) t) -> ('b,'e) t
Expand Down