fsprojects / FSharp.Control.TaskSeq

A computation expression and module for seamless working with IAsyncEnumerable<'T> as if it is just another sequence
MIT License
91 stars 7 forks source link

Cleanup code inspections / xmldoc rendering #222

Closed bartelink closed 8 months ago

bartelink commented 8 months ago

Calved from #220: small cleanups per Rider suggestions, and adding dictionary entries covering terms used in the repo.

Also:

abelbraaksma commented 8 months ago

EDIT: the below may not matter much after I found out that using IDisposable may actually be the preferred coding pattern in this particular case, and nothing of this is in public facing code, so I'm fine with the change


Original post:

direct thing to resolve it is to just not do that

Ehm, you mean to not use new, or to not implement/use IAsyncDisposable?

the Dispose impl is not doing Async things

but the IAsyncDisposable impl is doing Async things. In other words, the general rule "use new with disposable stuff holds for both IDisposable and IAsyncDisposable. Which means, if you use use inside a CE, it will dispose using the async method. If your implementation also supports non-async disposing, it may also implement IDisposable and work with use outside a CE block.

it's bad news that the tooling is suggesting dropping the new / not making the fact it's a disposable stick out.

Indeed, so we shouldn't drop new ;).

Granted, I may not get your point here, really 😆.

Related, this discussion shows that IAsyncDisposable should be preferred inside CE: https://github.com/dotnet/fsharp/issues/12563

I'll write something up to figure this out conclusively.

abelbraaksma commented 8 months ago

I've taken the liberty to squash these commits a bit, and fixed some small errors in xml doc tags, plus removed some "legalese" wording 😆. I think it is good to be merged now.

The idisposable part is now in #227 (which makes the branch name a little odd here, lol).

bartelink commented 8 months ago

Yes all these cleanup ones I'd prefer them to be squash merged (I'm a fan of the linear history tickbox)

abelbraaksma commented 8 months ago

squash merged (I'm a fan of the linear history tickbox)

Yeah, that's not supported on this repo, I do like to see informed and well-written commits. Squash-merging defeats that purpose. But I do keep a linear history, and require rebasing.