Skip to content

Add Lwt_retry library#1032

Merged
raphael-proust merged 11 commits into
ocsigen:masterfrom
shonfeder:1028/lwt-retry
Nov 4, 2024
Merged

Add Lwt_retry library#1032
raphael-proust merged 11 commits into
ocsigen:masterfrom
shonfeder:1028/lwt-retry

Conversation

@shonfeder

@shonfeder shonfeder commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

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.

@raphael-proust raphael-proust left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread lwt_retry.opam Outdated
Comment thread lwt_retry.opam Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.mli

@shonfeder shonfeder left a comment

Copy link
Copy Markdown
Contributor 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 careful review! I think I've addressed all the suggestions and corrections.

Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.ml Outdated
Comment thread src/retry/lwt_retry.mli
@raphael-proust

Copy link
Copy Markdown
Collaborator

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

@shonfeder

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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

you can go ahead @shonfeder

@shonfeder

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

Comment thread dune-project Outdated
Comment thread lwt_retry.opam Outdated
Comment thread test/retry/main.ml Outdated
@raphael-proust

Copy link
Copy Markdown
Collaborator

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

@shonfeder

Copy link
Copy Markdown
Contributor Author

Thank you for the fixes!

@raphael-proust raphael-proust merged commit b778004 into ocsigen:master Nov 4, 2024
@raphael-proust

Copy link
Copy Markdown
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