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

Binding options - allow system wide options #776

Merged
merged 2 commits into from
Dec 3, 2023
Merged

Conversation

RolandPheasant
Copy link
Collaborator

@RolandPheasant RolandPheasant commented Dec 3, 2023

Allow system wide binding option defaults + add capability to suppress reset for first time load,

Currently binding options like reset threshold can be specified at the bind operator. For example,

myObservableChangeSet.Bind(var out myCollection, resetThreshold:100);

The limitation here is you may want to specify system wide defaults, which will eliminate the need to specify the threshold on every bind instance. To solve this problem, static system wide options have been introduced.

DynamicDataOptions.Binding = new BindingOptions(100)

which means

myObservableChangeSet.Bind(var out myCollection);

will have a default threshold of 100, without the need to specify it.

There are additional options on BindingObject:

UseReplaceForUpdates - some control suites / platforms cannot handle replace events on the observable collection. If not true, replace and add new is used,
ResetOnFirstTimeLoad - similar to above, some control suites cannot handle a reset event so there's an option to prevent reset on first time load.

These options can be applied globally or at an individual bind level:

myObservableChangeSet.Bind(var out myCollection, new BindingOptions(100, ResetOnFirstTimeLoad : false);

// or to prevent reset completely and not just for the first time load
myObservableChangeSet.Bind(var out myCollection, new BindingOptions(100, BindingOptions.NeverFireReset());

Tests to follow, and there should be no breaking changes.

@RolandPheasant RolandPheasant marked this pull request as ready for review December 3, 2023 19:07
/// <returns>An observable which will emit change sets.</returns>
/// <exception cref="System.ArgumentNullException">source.</exception>
public static IObservable<IChangeSet<TObject, TKey>> Bind<TObject, TKey>(this IObservable<ISortedChangeSet<TObject, TKey>> source, out ReadOnlyObservableCollection<TObject> readOnlyObservableCollection, int resetThreshold = 25, bool useReplaceForUpdates = true, ISortedObservableCollectionAdaptor<TObject, TKey>? adaptor = null)
public static IObservable<IChangeSet<TObject, TKey>> Bind<TObject, TKey>(this IObservable<ISortedChangeSet<TObject, TKey>> source, out ReadOnlyObservableCollection<TObject> readOnlyObservableCollection, BindingOptions options)
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure about "Global" options but as long as the can be overwritten on a case-by-case basis, I'm good with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The global part sort of future proofs binding, Especially as machines are becoming more powerful it's good to be able to in one swoop increase the thresholds.

It's a shame all those other overloads have to be maintained but it would cause too much pain to remove them all. We should consider doing so on the next big version change.

public static IObservable<IChangeSet<T>> Bind<T>(this IObservable<IChangeSet<T>> source, IObservableCollection<T> targetCollection, BindingOptions options)
where T : notnull
{
if (source is null)
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with other changes, consider not using the braces here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment below

Copy link
Member

@dwcullop dwcullop left a comment

Choose a reason for hiding this comment

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

Other than using the braces in the null checks, I like everything about this.

Null checks can also be succinctly like:

    _ = parameter ?? throw new ArgumentNullException(nameof(parameter));

@RolandPheasant
Copy link
Collaborator Author

Other than using the braces in the null checks, I like everything about this.

Null checks can also be succinctly like:

    _ = parameter ?? throw new ArgumentNullException(nameof(parameter));

I would like an options which says to the style police to use either this format or a single line conditional without the braces. Both are fine with me, as I find using up the view port for null checks is needless. Originally the entire of DD was that way then I accepted a PR which introduced style cop which reformatted the entire thing to add all the braces, I did not realise this at the time and have been fighting with it ever since,

@ChrisPulman is there a way to tell style cop that single line conditionals are OK without braces? If so can we reformat all with the new rule?

@dwcullop
Copy link
Member

dwcullop commented Dec 3, 2023

@ChrisPulman is there a way to tell style cop that single line conditionals are OK without braces? If so can we reformat all with the new rule?

Actually, I just noticed that, after recent changes, StyleCop is suggesting the use of ArgumentNullException.ThrowIfNull(parameter); instead.

To me, it always makes sense to have the same pattern hidden behind an abstraction, even if it is very simple.

So, if there's going to be a mass replacement, my suggestion would be to use this instead.

@ChrisPulman
Copy link
Member

ArgumentNullException.ThrowIfNull(parameter);

In order to do this, we need a net4.6.2 / netstandard2.0 version of the function. I understand we just need a couple of attributes and a static method for ArgumentNullException.ThrowIfNull, but I can't be certain that these two frameworks have the ability to replicate the functionality.

@ChrisPulman
Copy link
Member

ArgumentNullException.ThrowIfNull(parameter);

In order to do this, we need a net4.6.2 / netstandard2.0 version of the function. I understand we just need a couple of attributes and a static method for ArgumentNullException.ThrowIfNull, but I can't be certain that these two frameworks have the ability to replicate the functionality.

With some research only netstandard2.1 supports the NotNull attribute so unfortunately we are stuck with the dual code block. We may be able to build an extension method that shortcuts the issue in a better way though, I will see if I can come up with something.

@JakenVeina
Copy link
Collaborator

Yeah, it's my understanding that we're not gonna be able to leverage stuff like ArgumentNullException.ThrowIfNull based on the frameworks we need to target, but there's still advantage to adding our own static helper methods. IIRC Stephen Toub talked about performance benefits of the pattern in his massive article on .NET 8 performance a few months ago.

https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-8/

@RolandPheasant
Copy link
Collaborator Author

Yeah, it's my understanding that we're not gonna be able to leverage stuff like ArgumentNullException.ThrowIfNull based on the frameworks we need to target, but there's still advantage to adding our own static helper methods. IIRC Stephen Toub talked about performance benefits of the pattern in his massive article on .NET 8 performance a few months ago.

https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-8/

It would be good to add our own to get around this, However, I wonder whether it's possible to run a formatter to change all the null reference exception checks with our code. Doing so would probably be difficult. Any ideas?

@RolandPheasant
Copy link
Collaborator Author

@dwcullop / @JakenVeina I've merged this, but will do some additional testing before release - I'll do a local package build and try it against some of my test / demo products,

@RolandPheasant RolandPheasant merged commit 3d4543f into main Dec 3, 2023
1 check passed
@RolandPheasant RolandPheasant deleted the feature/Binding branch December 3, 2023 22:38
@JakenVeina
Copy link
Collaborator

JakenVeina commented Dec 3, 2023

However, I wonder whether it's possible to run a formatter to change all the null reference exception checks with our code. Doing so would probably be difficult. Any ideas?

You kids and your aversions to just writing code. :P

I'm sure there are styling tools that'd do it, but I'm not familiar with any of them. I'll wager I can get nearly all of them with a good Regex Find/Replace. Make an issue so we don't forget, and I'd be happy to tackle it.

@glennawatson
Copy link
Member

Resharper and rider have tools for selecting one style

@RolandPheasant
Copy link
Collaborator Author

Ah yes, I used a custom reshaper refactoring to convert from nunit to xunit

@dwcullop
Copy link
Member

dwcullop commented Dec 4, 2023

If we can't use the standard static method, we should write our own (which could just invoke the standard one when building for a framework that supports it, maybe... Not sure what else the standard one does). Either way, it will be good to have all of the code using the same abstraction for this pattern.

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 Dec 19, 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.

5 participants