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

exposing SingleAssignmentDisposableValue and making it ICancelable #1630

Merged
merged 4 commits into from Apr 19, 2023

Conversation

Fijo
Copy link
Contributor

@Fijo Fijo commented Oct 30, 2021

When implementing various things outside of this library like a custom IScheduler or a custom rx operator the SingleAssignmentDisposableValue can be quite useful. Since it is already well documented and it seams to me like it can't be used wrong, I'd like to make it public and make implement ICancelable also.
Would that be ok with you?

Also please let me know if I have to bump versions manually.

@idg10
Copy link
Collaborator

idg10 commented Mar 31, 2023

Are you sure we should make it implement ICancelable? As far as I can tell, every endpoint that accepts an implementation of ICancelable would end up boxing this value, defeating the point of using a value type.

If there were cases where generic methods imposed a constraint, it might be different, but Rx doesn't appear to do that anywhere. So I'm wondering if the reason this type was defined as not implementing ICancelable was to make it less likely that people would accidentally box it by passing it to methods that accept an ICancelable.

It was pointed out by @idg10 that the ICancelable would promote accidental usages in scenarios that would result in the SingleAssignmentDisposableValue being boxed. I agree with that argument. As there's no real need to make it ICancelable I'm undoing that part of my change.
@Fijo
Copy link
Contributor Author

Fijo commented Apr 18, 2023

I agree with your argument. I'll refrain from making the SingleAssignmentDisposableValue implement ICancelable.

@idg10
Copy link
Collaborator

idg10 commented Apr 19, 2023

Thanks!

@idg10 idg10 merged commit 677aeb2 into dotnet:main Apr 19, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants