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 fix for .DisposeMany() #761

Merged

Conversation

JakenVeina
Copy link
Collaborator

Fixed that .DisposeMany() was not disposing items after downstream-teardown of the stream, I.E. unsubscription.

I was mistaken that onCompleted propagates to upstream observers. The mechanism for detecting downstream completion is the IDisposable that each subscription returns.

…teardown of the stream, I.E. unsubscription.
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 408 lines in your changes are missing coverage. Please review.

Comparison is base (53d5f6d) 64.74% compared to head (e633d8b) 65.65%.
Report is 4 commits behind head on main.

Files Patch % Lines
src/DynamicData/Cache/ObservableCacheEx.cs 53.48% 19 Missing and 1 partial ⚠️
src/DynamicData/List/ObservableListEx.cs 64.10% 13 Missing and 1 partial ⚠️
src/DynamicData/Aggregation/AvgEx.cs 73.68% 10 Missing ⚠️
src/DynamicData/Aggregation/SumEx.cs 74.35% 10 Missing ⚠️
src/DynamicData/ObservableChangeSet.cs 58.33% 10 Missing ⚠️
src/DynamicData/Platforms/net45/ParallelEx.cs 25.00% 7 Missing and 2 partials ⚠️
src/DynamicData/Aggregation/StdDevEx.cs 76.66% 5 Missing and 2 partials ⚠️
src/DynamicData/List/Internal/GroupOnProperty.cs 0.00% 7 Missing ⚠️
src/DynamicData/Aggregation/CountEx.cs 25.00% 6 Missing ⚠️
src/DynamicData/Cache/Internal/CacheUpdater.cs 53.84% 6 Missing ⚠️
... and 126 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #761      +/-   ##
==========================================
+ Coverage   64.74%   65.65%   +0.91%     
==========================================
  Files         226      227       +1     
  Lines       11459    11117     -342     
  Branches     2334     2294      -40     
==========================================
- Hits         7419     7299     -120     
+ Misses       3083     2882     -201     
+ Partials      957      936      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@RolandPheasant RolandPheasant left a comment

Choose a reason for hiding this comment

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

Hopefully this will do the job. @dwcullop can confirm by updating his PR to the latest branch after this is merged.

@JakenVeina JakenVeina merged commit 2a2757d into reactivemarbles:main Nov 21, 2023
3 checks passed
@JakenVeina JakenVeina deleted the dispose-many-downstream-cleanup branch November 21, 2023 10:13
Copy link

github-actions bot commented Dec 6, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants