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

Lwt_result.catch takes a function as parameter now #965

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

raphael-proust
Copy link
Collaborator

This PR is a work-in-progress. It should probably use a deprecated annotation instead, and introduce a new function to replace the existing one. Although:

@rgrinberg @hannesm @andersfugmann would you be ok with this breaking change of interface if I open a PR on your repos to adapt your code to match? Or do you need to stay compatible with multiple versions of Lwt for some packaging or dependency management reason?

@hannesm
Copy link
Contributor

hannesm commented Oct 12, 2022

I'm fine with your proposed change, thanks.

@rgrinberg
Copy link
Contributor

dune-rpc-lwt isn't used by anyone yet, so I'm fine with the breaking change.

@kohlivarun5
Copy link
Contributor

This is a breaking change, that has a considerable impact on internal code we have.
Either bumping the major version, or marking the previous one as deprecated and adding a new one would be more appropriate.

@raphael-proust
Copy link
Collaborator Author

@kohlivarun5 I did have concerns about changing the type like this. Thanks for the input! Out of curiosity: How much do you use Lwt_result? What about the other Lwt_* stdlib-like modules (Lwt_list, Lwt_seq, etc.)?

I'll try to find an appropriate new name to introduce. Any suggestion is welcome.

@raphael-proust
Copy link
Collaborator Author

opam grep to check the uses inside opam published packages:

$ opam grep 'Lwt_result\.catch' --depends-on=lwt
[Info] Getting the list of all known opam packages..
[Info] Fetching and grepping using rg..
aws-s3-async matches your regexp.
aws-s3-lwt matches your regexp.
aws-s3 matches your regexp.
dune-rpc-lwt matches your regexp.
lwt matches your regexp.
lwt_camlp4 matches your regexp.
lwt_domain matches your regexp.
lwt_ppx matches your regexp.
lwt_ppx_let matches your regexp.
lwt_react matches your regexp.
opam-compiler matches your regexp.
tezos-accuser-010-PtGRANAD matches your regexp.
tezos-baker-010-PtGRANAD matches your regexp.
tezos-baking-010-PtGRANAD-commands matches your regexp.
tezos-baking-010-PtGRANAD matches your regexp.
tezos-endorser-010-PtGRANAD matches your regexp.
tezos-endorser-alpha matches your regexp.
tezos-legacy-store matches your regexp.
zmq-lwt matches your regexp.

@andersfugmann
Copy link

andersfugmann commented Oct 13, 2022

I'm fine with the proposed change. It does mean that users of the aws-s3 will be forced to update lwt to access future bugfixes as I will not be backporting bugfixes.

Also fine wrt. zmq-lwt.

@kohlivarun5
Copy link
Contributor

@kohlivarun5 I did have concerns about changing the type like this. Thanks for the input! Out of curiosity: How much do you use Lwt_result? What about the other Lwt_* stdlib-like modules (Lwt_list, Lwt_seq, etc.)?

We use Lwt_result heavily (I would say 80% of the files rely on open Lwt_result.Syntax. We have some usage of Lwt_list, but more usage of Lwt_stream

I'll try to find an appropriate new name to introduce. Any suggestion is welcome.

I am also unsure about a new name, maybe catch_f

@raphael-proust
Copy link
Collaborator Author

@kohlivarun5 I've made a version that deprecates (but does not remove) the existing function. Let me know if you foresee any issue with releasing these changes as a minor bump.

Also, thank you for letting me know about library usage. :)

@MisterDA
Copy link
Contributor

MisterDA commented Oct 21, 2022

The _f seems a bit ugly to me 😄 what about catchf or fcatch or catchall? Maybe there's a precedent for _f that I don't know of. In truth all my proposals are quite ugly too.

@kohlivarun5
Copy link
Contributor

kohlivarun5 commented Oct 21, 2022

I looked at this again, and I was a bit mistaken before. I thought we were deprecating Lwt.catch.
We do not have much usage of Lwt_result.catch (we primarily use Lwt.catch)

So, we should be okay if you replace Lwt_result.catch with the new version 😄
I can see how it makes Lwt.catch and Lwt_result.catch more consistent

Apologies for the confusion here

@hannesm
Copy link
Contributor

hannesm commented Oct 21, 2022

Thanks for looking @kohlivarun5. I'm in favour, similar to @MisterDA to just redefine Lwt_result.catch with the new signature (ep. since it's not used too much). Bump the major version of Lwt, and users will adapt to that :)

@raphael-proust
Copy link
Collaborator Author

A new major version seems reasonable to me. Especially bc there are several PRs related to exception management. I'll go towards that.

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.

6 participants