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

Add Lwt_result.collect without deps #1040

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

seprov
Copy link

@seprov seprov commented Nov 15, 2024

What

This PR adds Lwt_result.collect without adding ocaml-containers as a dependency.

Why

To handle the common case where a list of results need to be collected to a single meaningful result.

Notes

partition_filter_map and split_result are available in ocaml-containers, but to avoid adding the dependency, I've recreated them here.

Also, I'm not sure how y'all like to mention future releases in documentation comments. Let me know about that, thanks.

Comment on lines 124 to 128
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.

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.

2 participants