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

Feature: EditDiff extension method for IObservable<Optional<T>> #739

Merged

Conversation

dwcullop
Copy link
Member

@dwcullop dwcullop commented Oct 14, 2023

Description

Similar to #738, this is an overload that operates on IObservable<Optional<T>> instead of IObeservable<IEnumerable<T>>. It is useful because it provides yet another way to have child-change sets (and also works nicely with MergeManyChangeSets as seen below).

When the first Optional with a value is observed, an Add change is emitted. When an empty Optional is observed, a Remove change is emitted for the previously Added value. If a non-empty Optional is observed, the Keys are computed and compared to determine if either a single Update (keys are the same) or a Remove/Add pair of Updates (keys are different) should be emitted.

Rationale

Since Optional is essentially a container with at most one value, the same behavior could be easily done with #738 and a helper function such as:

public static IEnumerable<T> AsEnumerable(this Optional<T> optional)
{
    if (optional.HasValue) yield return optional.Value;
}

public static IObservable<IChangeSet<TObject, TKey>> EditDiff<TObject, TKey>(this IObservable<Optional<TObject>> source, Func<TObject, TKey> keySelector, IEqualityComparer<TObject>? equalityComparer = null)
        where TObject : notnull
        where TKey : notnull =>
    source.Select(opt => opt.AsEnumerable()).EditDiff(keySelector, equalityComparer);

However, using such an approach will result in using an entire SourceCache that is essentially empty. This specialized version is optimized around only having to deal with a single value at a time and is extremely light weight.

Example

interface IPerson : INotifyPropertyChange
{
  int Id { get; }
  string Name { get; }
  Optional<IPerson> Spouse { get; }
  IReadOnlyList<IPerson> Children { get; }
}

IObservableCache<IPerson, int> peopleCache = GetPeopleCache();

// Treat all changes to all spouse properties as one observable changeset
// Creates "Add" events when a Spouse property is set with a value
// Creates "Remove" events when it is set without a value
IObservable<IChangeSet<IPerson, int>> spouseChangeSet = 
    peopleCache.Connect()
               .MergeManyChangeSets(person => person.WhenAnyValue(p => p.Spouse).EditDiff(p => p.Id));

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Merging #739 (08da7ce) into main (f1da1b1) will increase coverage by 0.20%.
Report is 2 commits behind head on main.
The diff coverage is 95.87%.

@@            Coverage Diff             @@
##             main     #739      +/-   ##
==========================================
+ Coverage   72.22%   72.43%   +0.20%     
==========================================
  Files         217      220       +3     
  Lines       11057    11154      +97     
==========================================
+ Hits         7986     8079      +93     
- Misses       3071     3075       +4     
Files Coverage Δ
...rc/DynamicData/Cache/Internal/EditDiffChangeSet.cs 100.00% <100.00%> (ø)
...icData/Cache/Internal/EditDiffChangeSetOptional.cs 97.87% <97.87%> (ø)
src/DynamicData/Cache/ObservableCacheEx.cs 52.24% <95.00%> (+0.69%) ⬆️
...DynamicData/Cache/Internal/ToObservableOptional.cs 90.90% <90.90%> (ø)

@dwcullop
Copy link
Member Author

I also have an implementation for the inverse of this operation that I am considering submitting because I find it to be very useful.

Something like:
IObservable<Optional<T>> ToObservableOptional<T, TKey>(this IObservable<IChangeSet<T, TKey>> source, TKey key);

It's similar to Watch except it converts to Optional (e.g., Remove events yield Optional.None).

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.

Makes sense. Thanks

@RolandPheasant
Copy link
Collaborator

@dwcullop annoyingly there's a merge conflict here.

@dwcullop
Copy link
Member Author

@dwcullop annoyingly there's a merge conflict here.

That's on me for submitting so many (similar) PRs at once, but I had to get them out while I had the inspiration. I'll resolve the conflicts, no worries.

@dwcullop
Copy link
Member Author

It worked out for the best anyway because the implementation was too complicated. The whole point of this specialization is that it can be lighter weight (compared to using the IEnumerable version), but it still ended up using a bunch of Rx operators that weren't strictly needed.

I updated the implementation to make it as thin as possible (only operator is Synchronize). It's a lot less code and is easier to read as well.

@RolandPheasant RolandPheasant merged commit 0be0d95 into reactivemarbles:main Oct 16, 2023
3 checks passed
@github-actions
Copy link

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 Oct 31, 2023
@dwcullop dwcullop deleted the feature/optional-editdiff branch November 23, 2023 00:57
@dwcullop dwcullop self-assigned this Dec 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants