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

Cleanup code inspections / xmldoc rendering #222

Merged
merged 2 commits into from
Dec 24, 2023

Conversation

bartelink
Copy link
Member

@bartelink bartelink commented Dec 23, 2023

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

Also:

  • Rider complains about the new on the IAsyncDisposable being redundant. In this instance, I think the most direct thing to resolve it is to just not do that - the Dispose impl is not doing Async things. But, as we discussed, it's bad news that the tooling is suggesting dropping the new / not making the fact it's a disposable stick out.
  • Rider complains about paramrefs that are not correct (dont reference actual things), but we are using them here as its the only way to have VS+VSCode highlight types or snippets in bold (and Rider does not render <c> in bold either)

@bartelink bartelink changed the title fix: Cleanup code inspections Cleanup code inspections Dec 23, 2023
@abelbraaksma
Copy link
Member

abelbraaksma commented Dec 23, 2023

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: dotnet/fsharp#12563

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

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.

This is good to go, once the changes are applied. Great work!!!

src/FSharp.Control.TaskSeq/TaskSeq.fsi Show resolved Hide resolved
src/FSharp.Control.TaskSeq/TaskSeq.fsi Outdated Show resolved Hide resolved
src/FSharp.Control.TaskSeq/TaskSeq.fsi Outdated Show resolved Hide resolved
src/FSharp.Control.TaskSeq/TaskSeq.fsi Outdated Show resolved Hide resolved
src/FSharp.Control.TaskSeq/TaskSeqInternal.fs Outdated Show resolved Hide resolved
@bartelink bartelink changed the title Cleanup code inspections Cleanup code inspections / xmldoc rendering Dec 23, 2023
@abelbraaksma
Copy link
Member

abelbraaksma commented Dec 24, 2023

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
Copy link
Member Author

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

@abelbraaksma
Copy link
Member

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.

@abelbraaksma abelbraaksma merged commit 9186f71 into fsprojects:main Dec 24, 2023
6 checks passed
@bartelink bartelink deleted the idisposable branch December 24, 2023 15:51
@abelbraaksma abelbraaksma added refactoring Cleanup, refactoring and minor fixes cleanup labels Dec 24, 2023
@abelbraaksma abelbraaksma added this to the v0.4.0 milestone Mar 20, 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