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_retry library #1032

Merged
merged 11 commits into from
Nov 4, 2024
Merged

Conversation

shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Oct 15, 2024

Adds the Lwt_retry library discussed in #1028 as a separate package. I believe all guidance suggested there by @raphael-proust has been incorporated. Thanks for that!

I did not add an iter function, since I think this doesn't add any value over using Lwt_stream.iter directly.

I've tried to make the code, documentation, and tests consistent with the style I found in other locations in the code base.

Copy link
Collaborator

@raphael-proust raphael-proust left a comment

Choose a reason for hiding this comment

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

Looks good. Some small nitpick-ish comments and a couple of actual suggestions. Feel free to push back though, I could very well have gotten something wrong.

lwt_retry.opam Outdated Show resolved Hide resolved
lwt_retry.opam Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.mli Show resolved Hide resolved
Copy link
Contributor Author

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

Thank you for the careful review! I think I've addressed all the suggestions and corrections.

src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.ml Outdated Show resolved Hide resolved
src/retry/lwt_retry.mli Show resolved Hide resolved
@raphael-proust
Copy link
Collaborator

Thanks! I'll have a look at the dune version thing this week

@shonfeder
Copy link
Contributor Author

Rebased on #1035 so I could update the package author and maintainer data. I'll need to rebase on master once that is merged in.

The only CI failures remain the FreeBSD and MacOS failures that are occurring in the trunk:

Test 'getgrgid and Unix.getgrgid' in suite 'lwt_unix' raised 'Not_found'
Test 'getgrnam and Unix.getgrnam' in suite 'lwt_unix' raised 'Not_found'

@raphael-proust
Copy link
Collaborator

I'll need to rebase on master once that is merged in

you can go ahead @shonfeder

@shonfeder
Copy link
Contributor Author

Rebased! Should be good for a final review and/or merge! Thanks again for the reviews and guidance along the way, @raphael-proust :)

@shonfeder
Copy link
Contributor Author

Anything else you'd like to see here @raphael-proust ?

dune-project Outdated Show resolved Hide resolved
lwt_retry.opam Outdated Show resolved Hide resolved
test/retry/main.ml Outdated Show resolved Hide resolved
@raphael-proust
Copy link
Collaborator

Ready to merge unless the CI shows I messaed up something with my suggested+applied changes

@shonfeder
Copy link
Contributor Author

Thank you for the fixes!

@raphael-proust raphael-proust merged commit b778004 into ocsigen:master Nov 4, 2024
26 of 28 checks passed
@raphael-proust
Copy link
Collaborator

Thanks for the contribution!!

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