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

Changed parameter inside Ensure/EnsureFor for nested objects #151

Merged
merged 3 commits into from
Dec 25, 2017

Conversation

GooRiOn
Copy link
Member

@GooRiOn GooRiOn commented Dec 13, 2017

Info

This Pull Request is related to issue no. #147

So, as we discussed in the issue, the IValitRulesProvider<TObject> parameter for validating nested object was replaced with IValitator<TObject> type. Code is now clearer and honestly makes more sense. For validation we provide object responsible for validation instead of rules provider. Imho, provider should be used to instantiate new IValitRules<TObject> like:

ValitRules<Model>
    .Create(_provider.GetRules())
....

But of course can be still transformed into valitator using CreateValitator() extension method (so the breaking change isn't that big).

Changes

  • Replaced parameters inside Ensure and EnsureFor for nested objects to accepts IValitator<TObject> type instead
  • Changed implmentation of NestedObjectValitRule and NestedObjectCollectionValitRule to accept IValitatot<TObject> in the constructor
  • Changed unit tests

@codecov
Copy link

codecov bot commented Dec 13, 2017

Codecov Report

Merging #151 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #151   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           39     39           
  Lines          763    763           
======================================
  Hits           763    763
Impacted Files Coverage Δ
src/Valit/ValitRules.cs 100% <100%> (ø) ⬆️
src/Valit/Rules/NestedObjectValitRule.cs 100% <100%> (ø) ⬆️
src/Valit/Rules/NestedObjectCollectionValitRule.cs 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7cac04...72da760. Read the comment docs.

timdeschryver
timdeschryver previously approved these changes Dec 14, 2017
Copy link
Collaborator

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

LGTM, totally agreeing with you!
Maybe we can go a step further?
What I mean by this is, we could remove the IValitRulesProvider (and making GetAllRules() private?), in favor of ValitRules if possible.

public IEnumerable<IValitRule<NestedModel>> GetRules()
                => ValitRules<NestedModel>
                        .Create()
                        .Ensure(m => m.NumericValue, _ => _
                            .IsGreaterThan(10)
                                .WithMessage("One"))
                        .Ensure(m => m.StringValue, _ => _
                            .Required()
                                .WithMessage("Two")
                            .Email()
                                .WithMessage("Three"))
                        .GetAllRules();

would become

public ValitRules<NestedModel> GetRules()
                => ValitRules<NestedModel>
                        .Create()
                        .Ensure(m => m.NumericValue, _ => _
                            .IsGreaterThan(10)
                                .WithMessage("One"))
                        .Ensure(m => m.StringValue, _ => _
                            .Required()
                                .WithMessage("Two")
                            .Email()
                                .WithMessage("Three"));

@GooRiOn
Copy link
Member Author

GooRiOn commented Dec 15, 2017

@tdeschryver but how would you get the rules from IValitRules object having mentioned methods private?

@timdeschryver
Copy link
Collaborator

Oh did I say private? I meant internal 😅 , so we could access the GetRules() on validation.

@GooRiOn
Copy link
Member Author

GooRiOn commented Dec 17, 2017

@tdeschryver but the idea behind provider was to have a structure responsible for providing ruleset. So, you can get them and create a new instance of IValitRules. Imagine situation when you have the following classes:

class A
{
     public string Value {get; set;}
}

class B : A
{
    public int ConcreteValue {get; set;}
}

With a provider you could create a rules just for the Value property for A type. Then somehere in you code, you could get these rules and create new instance of IValitRules<B> :

ValitRules<B>
    .Create(ATypeRules)
    .Ensure(m => m.ConcreteValue, ...)

So you follow DRY instead of typing same rules for the concrete type.

@GooRiOn
Copy link
Member Author

GooRiOn commented Dec 17, 2017

Oh wait, I know what you mean now :D This may be a good decision since there would be no confusion what actually GetRules returns... IValitRules or enumerable of rule... But that's another breaking change :/

@timdeschryver
Copy link
Collaborator

@GooRiOn True, but we still can take advantage that we aren't in v1? 😄

@GooRiOn
Copy link
Member Author

GooRiOn commented Dec 17, 2017

@tdeschryver I mentioned that in #145 ;)

@GooRiOn
Copy link
Member Author

GooRiOn commented Dec 25, 2017

@tdeschryver I'll merge this and create new issue for IValitRulesProvider topic since I don't want to block this change ;)

@GooRiOn GooRiOn added this to the 1.3.0 milestone Dec 25, 2017
@GooRiOn GooRiOn merged commit 04d2b49 into develop Dec 25, 2017
@GooRiOn GooRiOn deleted the feature/147 branch December 25, 2017 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants