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

Typos / tweaks #125

Merged
merged 6 commits into from
Dec 16, 2022
Merged

Typos / tweaks #125

merged 6 commits into from
Dec 16, 2022

Conversation

bartelink
Copy link
Member

  • Typos
  • Remove Program.fs from test projects
  • Fix test namespace
  • Remove helpers using Async.AwaitTask

@bartelink
Copy link
Member Author

bartelink commented Dec 11, 2022

The removal of the Async.AwaitTask-based helpers is based on the fact that

@abelbraaksma
Copy link
Member

Thanks for this and for fixing the typos.

Those helpers are not used, because they are part of the surface area (i.e., the exposed functions). They should have tests attached to them, probably. Async.toTask is there to help use the lib, esp. as long as let! x = async {...} isn't supported. The inverse is there to round up the helpers (i.e., if you have an ofXXX, I tend to add the toXXX if possible).

We are not bound to the backwards and binary compat issues that F# needs to abide by, so maybe we should just use the "safe version" that's mentioned in your linked issue.

Also, I'm aware of AggregateException wrapping, which is why I have a helper test function to cover that stuff (it happens also in other scenarios).

@abelbraaksma abelbraaksma added refactoring Cleanup, refactoring and minor fixes cleanup labels Dec 12, 2022
@bartelink
Copy link
Member Author

I could repurpose this PR by:

  • remove the renamespacing commit (I see you have it in your other PR)
  • add AwaitTaskCorrect (I'll put in the canonical version as per the fssnip, as used in Equinox, Propulsion and FSharp.AWS.DynamoDB)
  • reinstate the stuff using AwaitTask but using ATC instead

I'd propose to leave the AwaitTaskCorrect as private as egregious use of this nuget to patch stuff is not going to end well for anyone

I have some concerns about Task/Async.map - it's presence rarely does good things for code IME even though I know lots of people want it, and it logically fits in with a certain style - would be happy to move it to the test module...

Regarding AggregateException, if you have time, I have a puzzler I posed in https://discord.com/channels/196693847965696000/196695876054286336/1051212510675607552 - unfortunately I don't understand it well enough / havent worked hard enough to distil the problem down as yet to identify where it's coming from/being introduced (e.g. it could be doubly nested coming from the AWS SDK and two layers of AwaitTaskCorrect Flattening transitions might have been neutralising that)

@abelbraaksma
Copy link
Member

I have some concerns about Task/Async.map - it's presence rarely does good things for code IME even though I know lots of people want it, and it logically fits in with a certain style - would be happy to move it to the test module...

I see your point. I kinda figured these were nice to have standard functions. Maybe I should remove all the Async/Task/ValueTask helpers (except for ValueTask.CompletedTask, it is needed because NetStandard 2.1 doesn't have it).

But let's take this discussion separate from this PR. Currently there isn't a single lib that has a bunch of these helper functions around, each lib comes with some of their own (fstoolkit, F#+, FusionTasks, AsyncEx), or none. Me adding to that forest of confusion is probably not a good thing.

Maybe I should just collect all the useful helpers and create them as a separate lib, and internalize them here as private.

@bartelink
Copy link
Member Author

OK, force pushed shaving off the commits re renamespacing and removal of AwaitTask-dependent stuff, so this is ready for merging and/or review (let me know if you want me to re-add the renamspacing of .Test -> .Tests)

@bartelink
Copy link
Member Author

bartelink commented Dec 13, 2022

But let's take this discussion separate from this PR. Currently there isn't a single lib that has a bunch of these helper functions around, each lib comes with some of their own (fstoolkit, F#+, FusionTasks, AsyncEx), or none. Me adding to that forest of confusion is probably not a good thing.

Maybe I should just collect all the useful helpers and create them as a separate lib, and internalize them here as private.

I'd dearly love to see an Task.toAsync and/or Async.ofTask (which AFAICT nicely sidesteps the AwaitTaskCorrect naming vs breaking changes to Async.AwaitTask breaking changes problems) that addresses fsharp/fslang-suggestions#840. If that can't be in FSharp.Core (the fact that it would be v 6.0.8 or 6.1.0 would be a bit ugly too, considering my complaining about 6.0.2 requirements!), a new lib would be good (with a pretty strict terms of reference so it does not become a dumping ground for random low value Task/Async helpers). Even if it was in a separate lib, I'd have Equinox, Propulsion and FsKafka take a dependency on it as the current inlining is a really ugly state of affairs as it is. That would pave the way for general guidance and/or FSharpLint to outlaw Async.AwaitTask.

NOTE that working around/shimming some ignore and/or StartImmediateAsTask helpers in the context of a lib is nowhere near the same level of annoyance and confusion as having to cart around AwaitTaskCorrect.

I guess that issue is probably the place to have the full discussion.

@abelbraaksma
Copy link
Member

I've rebased this. The remaining changes here are still useful, let's get them in. Tx for the cleanup!

Copy link
Member

@abelbraaksma abelbraaksma left a comment

Choose a reason for hiding this comment

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

LGTM. I'll make one small change and will merge. The big change suggestion will go in a new PR.

src/FSharp.Control.TaskSeq/FSharp.Control.TaskSeq.fsproj Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@abelbraaksma abelbraaksma merged commit 08d8e99 into fsprojects:main Dec 16, 2022
@abelbraaksma
Copy link
Member

Thanks for finding and fixing all my spelling errors, and improving the language!

@bartelink bartelink deleted the typos branch December 16, 2022 08:25
README.md Show resolved Hide resolved
@abelbraaksma abelbraaksma added this to the v0.4.0-alpha.1 milestone Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup refactoring Cleanup, refactoring and minor fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants