michaellperry / Assisticant

MIT License
36 stars 19 forks source link

Support for INotifyDataErrorInfo on wpf #22

Closed half-evil closed 6 years ago

half-evil commented 7 years ago

Hey Michaell,

thank you for this cool lib. I've always searched for a knockout-style lib for wpf. After diving in your code I've found out, that the PlatformProxy<> on the WPF-platform only supports IDataErrorInfo. INotifyDataErrorInfo is only supported on the Universal-Platform. Is there a possibility to get a PlatformProxy<>which implements INotifyDataErrorInfo without recompiling Assinsticant? It would be cool to have the choice which interface my viewmodels are wrapped in.

Greetings, Christian

michaellperry commented 7 years ago

Good idea, Christian. At the time I created the WPF library, INotifyDataErrorInfo was only available in Silverlight. It won't take long to publish an update.

michaellperry commented 7 years ago

I created a branch called "errorinfo" to work on this. Rather than simply pass through to the view model, I'm planning on using dependency tracking to notify on your behalf. Here's the current idea:

public class MyViewModel : IValidation
{
    public ValidationRules Rules => new ValidationRules()
        .For(() => MyProperty, x => x
            .Required()
            .MaxLength(25))
        .For(() => MyOtherProperty, x => x
            .Matches("^[a-zA-Z0-9]*$"));
}

Feedback?

michaellperry commented 7 years ago

Here's an initial sketch of the idea. https://github.com/michaellperry/Assisticant/tree/errorinfo

half-evil commented 7 years ago

Hi Michael, sorry for my late response, but I got sick lately. Thank you for this fast implementation! I've tested it in one of my wpf-apps and it's working well. The fluent api is nice and it is an good starting point for implementing custom validation rules. But in vb.net I don't get code completion for x (StringPropertyValidationRule), although VS shows the right type when I hover over x with the mouse. Even after implementing a custom PropertyValidationRule and an extension method, I don't get code completion. Do you have an idea?

After compiling and using the errorinfo branch, I hit Debug.Assert(false, "An observable was changed while updating a computed."); in Observable.cs an line 142. The error occurs when I add items to an ObservableList in my VM. Is this because I'm using Observables in my viewmodels instead of plain types for binding data? What is the preferred way? In updatecontrols it was Observables in VM and poco models.

Greetings, Christian

michaellperry commented 7 years ago

Thanks for giving it a try.

First, I also see the type inference problems. Perhaps the rule syntax asks too much of the languages. I'll see if I can find a friendlier syntax.

Second, "Observable changed while updating" occurs when you change an Observable in response to a change in another. My guess is that you are creating an ObservableList as a method level variable. ObservableList is intended to be used in fields, and particularly in models instead of view models. If this doesn't help, post some code and I'll see if I can figure it out with you.

michaellperry commented 7 years ago

Being explicit about the property types solves the type inference problems with intellisense. It also leaves the door open for more generic validation rules. See my recent commit.

michaellperry commented 7 years ago

I added a method for generic validation rules. Supply a predicate and a string.

    public ValidationRules Rules => new ValidationRules()
        .For(() => Death,
            v => v == null || v > Birth,
            () => "Death date must be after birth date.");

Even better if the string is from a localized resource.

Notice how this lets you mix multiple properties of the view model. You just have to declare which property the error is attached to. You can even ignore the lambda parameter if that makes it more readable:

    public ValidationRules Rules => new ValidationRules()
        .For(() => Death,
            _ => Death == null || Death > Birth,
            () => "Death date must be after birth date.");

I'm considering whether I want to do some kind of .When() or .Unless() syntax like Fluent Validation.

    public ValidationRules Rules => new ValidationRules()
        .For(() => Death,
            _ => Death > Birth,
            () => "Death date must be after birth date.")
            .WhenNotNull();

Should I just tie in to Fluent Validation? Or should I just skip the fluent syntax and let you express predicates using the language?

floyd-may commented 7 years ago

I would "second" making it easy to adapt to Fluent Validator rather than trying to reproduce its API. Maybe something like:

public IValidationRules Rules =>
    ValidationRules
        .ForEntity(() => _myFluentValidator.Validate(this));

As for the API you've got ATM in the errorinfo branch, might I suggest a couple changes?

First, make IValidation look like this:

public interface IValidation
{
    IValidationRules Rules { get; }
}

This will free you up to do something like this:

public IValidationRules Rules =>
    ValidationRules
    .For(() => PhoneNumber)
        .Where(r => r.Matches(@"^[0-9\-\(\)]*$"))
        .Where(r => r.Length > 10)
        .WithMessage("Phone Number must be reasonable.")
    .For(() => Age)
        .Where(r => r.GreaterThanOrEqualTo(0))
    .For(() => Age)
        .Where(r => r.LessThan(150))
    .For(() => Death)
        .Where(v > Birth)
        .IfNotNull()
        WithMessage("Death date must be after birth date.");

This way you can, for each property you're validating, see the rules for it based on how you indent it. (I'm a big fan of indentation conventions in fluent APIs)

I'm also a big fan of a static entry point for fluent APIs (ValidationRules.For(...) instead of new ValidationRules().For(...)), less syntax to mentally parse IMHO.

You can also match the property types and return a different concrete type from each For(...) method without having the extra verbosity of ForInt(...) versus ForString(...), etc. As long as the return type of the first .Where(...) implements IValidationRules you should be golden.

If I were to try to wrangle some code together for this, should I target the errorinfo branch for a PR?

michaellperry commented 7 years ago

Yes, please. Target the errorinfo branch.

michaellperry commented 6 years ago

Validation has been merged. Thanks @floyd-may!

floyd-may commented 6 years ago

Glad to help! This was a fun api to build.

On Fri, May 11, 2018, 5:31 PM Michael L Perry notifications@github.com wrote:

Validation http://assisticant.net/validation.html has been merged. Thanks @floyd-may https://github.com/floyd-may!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/michaellperry/Assisticant/issues/22#issuecomment-388501572, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiMNJXXWoPvc2ZxR4e6g1d46vCDZLclks5txhFXgaJpZM4L3mTw .