-
Notifications
You must be signed in to change notification settings - Fork 27
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
Possible incorporation into FSHarp.Core #27
Comments
Just to say this work is now ready for review, I'd be really grateful if people experienced with using TaskBuilder.fs could read the RFCs and review the implementation dotnet/fsharp#6811 For example, should FSharp.Core come with things like |
Also to mention that I removed the use of complex SRTP successfully replaced by a set of method overloads, see https://github.com/dotnet/fsharp/blob/feature/tasks/src/fsharp/FSharp.Core/tasks.fsi Also see the RFC section https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1097-task-builder.md#bind-returnfrom-support @rspeele Do you recall if there was a specific reason to use the more complex form of SRTP, with multi-parameter witnesses, priorities etc.? Beyond the need to resolve method overloading order? |
Hey, I think how you have it now, using extension methods in modules opened highest-priority-last, is how TaskBuilder used to work back when I last significantly contributed to it. Something like this version. TaskBuilder in this form was, unbeknownst to me, dependent on a subtle change to type inference introduced in F# 4.1. When others tried to use it with the F# 4.0 compiler they got type inference errors and ambiguity over which Bind or and ReturnFrom should be selected. @gusty swooped in to the rescue (thank you!) and added the SRTP magic to make it work as it does now, which plays nicely with both old and new versions of the compiler. For purposes of a new addition to F#, compatibility with the old compiler might not be so relevant. As long as the type inference behavior stays consistent and keeps this simple module-priority extension method technique working, I personally think it's simpler and easier to understand. This old issue and the ones linked from it is a good reference. |
@dsyme Nice job ! I like explicit things, like |
@dsyme that's a good question. I understand the value of aligning with Async, but at the same time I think Async type is a bit special in the sense that all methods are Pascal case, because of the overloads, which require method instead of functions. My personal preference is not to follow that design and align instead with the rest of the FSharp.Core modules. When there are cases which requires overloads we can define different function names. An alternative to that would be use overloads but with camel case, you'll be breaking the naming conventions, but actually this reveals a pain point in the naming conventions in that the casing expose what I consider implementation details: module function or type member. And as I said before, I'm not a big fan of ad-hoc overloading as their reasoning/understanding relies more on tooling and there are situations where they don't compose well. I saw your job with Async2 maybe that will be a good opportunity to align the Async module. Here's F#+ Task module, it's really a tiny set of functions, not saying that it's complete, more functions can be added: https://github.com/fsprojects/FSharpPlus/blob/master/src/FSharpPlus/Extensions/Task.fs Finally I insist in the importance of being explicit when switching from Async and Task, for different task-like awaitables it is ok as we can consider them a set of the same abstraction. |
It's certainly an important design point to consider. We never wrote out a rationale for PascalCasing of Async. Here are my recollections for the record
Of these points 1, 2, 3 and 4 still hold to some extent and also apply to task. We did add It would be lovely if we could somehow move all of this out of FSharp.Core, or split/refactor FSharp.Core. I notice FSharpPlus Async.fs as an alternative viewpoint, though there's not a lot there. I'd imagine we won't act on this as part of RFC FS-1097, but we should open a language suggestion for future Task/Async modules or classes. |
For me I think it's pretty much ok to implicitly bind an Async in a task. It's essentially foregoing the multi-start and cancellation token passing of the Async The other way around should be explicit with AwaitTask since the cancellation token is lost |
@dsyme thanks for sharing your thoughts, very interesting. We should certainly open a suggestion for re-organizing F# core. Even if we manage to move async to a module and use camel case names, we should review F# design guidelines, as there it states that methods should be Pascal case, as I said sometimes it is just an implementation detail.
That would be my preferred way. I am OK with overloads as long as they represent the same abstraction, but I consider Task-likes and Async two different abstractions as although they address the same problem, the models differ a lot. Also think about this if task accepts implicitly async, the expectation is that the opposite should also be true and it's not. So, better to be consistent. |
I'm looking at an incorporation into FSHarp.Core, see dotnet/fsharp#6811
Please let me know what you think, please follow that thread and please look at the design carefully. At the moment it follows the same general pattern as TaskBuilder.fs and the implementation is based on it.
Ply is also relevant, https://github.com/crowded/ply/, and we need to do state machine generation as well
The text was updated successfully, but these errors were encountered: