-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 F# and VB.NET samples #2046
Conversation
Relates to App-vNext#2045.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2046 +/- ##
=======================================
Coverage 83.67% 83.67%
=======================================
Files 312 312
Lines 7105 7106 +1
Branches 1054 1054
=======================================
+ Hits 5945 5946 +1
Misses 789 789
Partials 371 371
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Add documentation for using Polly with F# and VB.
Add some additional terminology.
Thoughts @martintmk and @peter-csala? |
Make the sample for using Polly from F# more idiomatic for those users. Co-Authored-By: Stuart Lang <stuart.b.lang@gmail.com>
I'm not a VB or F# expert but I think we should either add direct support to these languages or say that they are not supported. We can't expect that everyone will read our samples and apply the I'm not even sure that this solution would work for more complex scenarios as well. The sample codes are too simple to make a fair judgement whether this workaround is applicable everywhere. |
I wouldn't say it's not supported, it's just difficult. We did however neglect to consider the impact on VB.NET and F#, which is our bad, but given we're not familiar with those languages in the first place, I don't think it's a failure on our part.
I asked @slang25 who knows F# well to review my initial sample, and it seems to be fairly trivial to make it work compared to VB (adding I intentionally left them simple so that someone who considers themselves an "expert" can make their own judgement on whether they want to use it that way or not if they read the documentation. A VB guru might just say to themselves "this is too hard" and not use it, which is fine, if unfortunate. A "novice" might decide they're fine if they need to use it in one place and haven't ever used Polly before. The "this is too hard" outcome is what the original user who brought this to my attention went with. As noted in the PR comment, we could consider adding some extension methods that do the I don't think there's otherwise anything we can do about it other than accept it's not a good experience in those languages and that maybe one day F# will catch up to C# in terms of native |
I am worried tha adding these extensions might introduce ambiguity. If you remember, at some point, we actually had these but decided to drop them in favor of having Since the VB and F# is not our main scenario. how about we make the life to these consumers easier by introducing these extensions in something like |
Yeah, I was thinking about putting them in a new package rather than in Core. Was more the API shape idea I was looking for feedback on initially. |
Does it impact only the |
Any I'm not suggesting we augment every single API - just the core/basic use cases. |
All the above listed properties are methods that are returning either Because of that, I can do this: Dim options As New TimeoutStrategyOptions With {.Timeout = TimeSpan.FromSeconds(1),
.OnTimeout = Function(args)
Console.WriteLine("Timed out")
Return New ValueTask()
End Function} but I can't do this Dim options As New TimeoutStrategyOptions With {.Timeout = TimeSpan.FromSeconds(1),
.OnTimeout = Async Function(args)
Await Task.Delay(1000)
Console.WriteLine("Timed out")
End Function}
|
Out of the box F# cannot create ValueTasks with nice syntax, but it can consume them just fine. For creation, there are a few ways to get to something usable. Here's a quick sample showing my two preferred ways: module Polly.FSharp
open Polly
open System
open System.Threading
open System.Threading.Tasks
let pipeline =
ResiliencePipelineBuilder()
.AddRetry(Retry.RetryStrategyOptions())
.AddTimeout(TimeSpan.FromSeconds 5.)
.Build()
let token = CancellationToken.None
// this creates a Task using the F# built-in Task builder, then creates a valueTask from it
// this is more allocate-y but works out of the box. The default 'task' builder ends up returning a Task<int> at the top-level
let builtinValue =
task {
let! result = pipeline.ExecuteAsync((fun token -> (task { return 42 }) |> ValueTask<int>), token)
return result
}
// this uses the valueTask builder from IcedTasks to have 'direct' use of ValueTasks
// lower allocations, but requires adding a library (though one that is pretty widely used in the F# community).
// this version ends up returning a ValueTask<int> at the top-level.
// `dotnet add package IcedTasks`
open IcedTasks
let vTaskResult =
valueTask {
let! result = pipeline.ExecuteAsync((fun token -> valueTask { return 42 }), token)
return result
}
I also wanted to show the difference in the 'outer' experience for the user. Here's a screenshot from my editor to illustrate: With the 'native' solution, you are working with Task<'t> externally - the |
Apply feedback to use IcedTasks to consume ValueTask in the C# sample. Co-Authored-By: Chet Husk <573979+baronfel@users.noreply.github.com>
@baronfel @TheAngryByrd I've updated the F# sample to use IcedTasks as suggested - let me know if there's anything further that can be tightened up. |
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.
Looks good from F# point of view :)
let getBestFilmAsync token = | ||
task { | ||
do! Task.Delay(1000, token) | ||
return "https://www.imdb.com/title/tt0080684/" |
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.
Good choice :)
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.
Oops after I comment I found something. This has an extra valueTask wrapper in places that doesn't need to be here.
samples/Intro.FSharp/Program.fs
Outdated
do! | ||
valueTask { | ||
do! pipeline.ExecuteAsync( | ||
fun token -> | ||
valueTask { | ||
printfn "Hello, world! Waiting for 2 seconds..." | ||
do! Task.Delay(1000, token) | ||
printfn "Wait complete." | ||
} | ||
, token | ||
) | ||
} |
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.
Oops after I comment I found something. This has an extra valueTask
wrapper that doesn't need to be here.
do! | |
valueTask { | |
do! pipeline.ExecuteAsync( | |
fun token -> | |
valueTask { | |
printfn "Hello, world! Waiting for 2 seconds..." | |
do! Task.Delay(1000, token) | |
printfn "Wait complete." | |
} | |
, token | |
) | |
} | |
do! pipeline.ExecuteAsync( | |
fun token -> | |
valueTask { | |
printfn "Hello, world! Waiting for 2 seconds..." | |
do! Task.Delay(1000, token) | |
printfn "Wait complete." | |
} | |
, token | |
) |
samples/Intro.FSharp/Program.fs
Outdated
let! bestFilm = | ||
valueTask { | ||
let! url = pipeline.ExecuteAsync((fun token -> valueTask { | ||
let! url = getBestFilmAsync(token) | ||
return url | ||
}), token) | ||
return url | ||
} |
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.
let! bestFilm = | |
valueTask { | |
let! url = pipeline.ExecuteAsync((fun token -> valueTask { | |
let! url = getBestFilmAsync(token) | |
return url | |
}), token) | |
return url | |
} | |
let! bestFilm = | |
pipeline.ExecuteAsync((fun token -> valueTask { | |
let! url = getBestFilmAsync(token) | |
return url | |
}), token) |
Remove `valueTask` wrappers that are redundant. Co-Authored-By: Jimmy Byrd <1490044+theangrybyrd@users.noreply.github.com>
Thanks for the feedback everyone. The docs are now live here: https://www.pollydocs.org/use-with-fsharp-and-visual-basic.html |
Issue #2045 pointed out that the usage for Polly v8 isn't as terse as it is for C# due to the inability to directly await
ValueTask{<T>}
.This PR adds some limited F# and VB.NET examples to the documentation that demonstrate how to bridge the gap between
ValueTask
andTask
for Polly v8 pipelines.I also wonder if in a separate PR in a future release, we could maybe introduce some extension methods to make interop with
Task
easier as an opt-in at the expense of allocating more?Something like this:
Then it could be consumed like this without needing to use
AsTask()
or return aValueTask
from the user's callback: