-
Notifications
You must be signed in to change notification settings - Fork 590
Change GitHub.createPullRequest to await the call to PullRequest.Create #2821
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
base: master
Are you sure you want to change the base?
Conversation
Indeed, that would be strange |
Given that this is a breaking API change, would it need to be held for the .NET 8 update release? |
That would be more appropriate. |
Ok, I'll leave until the .NET 8 changes are done |
77dc4e4
to
72aeb27
Compare
Should we already merge this? |
Is there going to be a 6.1.2 with any of the other more minor changes, or is just in to version 8 now? Also needs a release note entry |
@@ -469,4 +469,4 @@ module GitHub = | |||
/// </example> | |||
let createPullRequest owner repoName (pullRequest: NewPullRequest) (client: Async<GitHubClient>) = | |||
retryWithArg 5 client | |||
<| fun client' -> async { return Async.AwaitTask <| client'.PullRequest.Create(owner, repoName, pullRequest) } | |||
<| fun client' -> async { return! Async.AwaitTask <| client'.PullRequest.Create(owner, repoName, pullRequest) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to wrap that into async
CE?
Description
Referencing the comments in #2820, and assuming that the return type is actually wrong, I think it should be awating the
PullRequest.Create
so that it gets awaited inside the retry machinery?Still a draft, as if it's a correct cfhange it needs release notes (and It'd be a breaking change to the public API)