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

Observable comparers for SortAndBind #884

Merged
merged 2 commits into from
Mar 24, 2024
Merged

Conversation

RolandPheasant
Copy link
Collaborator

@RolandPheasant RolandPheasant commented Mar 23, 2024

Add IObservable<IComparer> option to SortAndBind to enable changing the sort order.

var comparer = SortExpressionComparer<Person>.Ascending(p => p.Name).ThenByAscending(p => p.Age);
var comparerObservable = new BehaviorSubject<IComparer<Person>>(comparer );

var sortAndBind = _cache.Connect()
            .SortAndBind(out var list, comparerObservable)
            .Subscribe()

This represents the completion of SortAndBind.

@dwcullop / @JakenVeina

I know there were some previous conversations about SubscribeSafe but I skipped over that. My question is why has the pattern of .SubscribeSafe(Observer.Create... been introduced and when should it be applied? I ask because I used SubscribeSafe(observer) in this PR

@RolandPheasant RolandPheasant changed the title [WIP] Observable comparers for SortAndBind Observable comparers for SortAndBind Mar 23, 2024
@JakenVeina
Copy link
Collaborator

My question is why has the pattern of .SubscribeSafe(Observer.Create... been introduced and when should it be applied?

Bottom line is it's recommended when doing subscriptions within operators.

Subscribes to the specified source, re-routing synchronous exceptions during invocation of the System.IObservable1.Subscribe(System.IObserver{0}) method to the observer's System.IObserver`1.OnError(System.Exception) channel. This method is typically used when writing query operators.

In a practical sense, I tried to write up a small test scenario that shows this in action, and after an hour, I gave up. There's a LOT of baked-in safeguards within RX that normally make .SubscribeSafe() redundant. I do remember this was part of the solution for #668. I also suspect that using .SubscribeSafe() over .Subscribe() has no observable effect on performance, because the meaningful difference between the two is that .SubscribeSafe() does 1 or 2 extra if checks, on top of the many that .Subscribe() already does. Good news is we can easily benchmark this, if we want to be sure, now that we have a few operators that use .SubscribeSafe() and also have benchmarks written for them.

where TObject : notnull
where TKey : notnull
{
// NB: Either comparer or comparerChanged will be used, but not both.
Copy link
Collaborator

Choose a reason for hiding this comment

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

NB?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL


// sort is not returning uniqueness
if (insertIndex < 0)
private int GetInsertPositionBinary(TObject item)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, this diff is obnoxiously terrible in GitHub, BTW. The syntax-aware diff in VS is so much better. Never seen a case this bad before.

@RolandPheasant RolandPheasant merged commit 6f45aa3 into main Mar 24, 2024
1 check passed
@RolandPheasant RolandPheasant deleted the feature/changet-sort branch March 24, 2024 06:18
Copy link

github-actions bot commented Apr 8, 2024

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 Apr 8, 2024
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