-
Notifications
You must be signed in to change notification settings - Fork 26
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
Refactored validation part #152
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #152 +/- ##
======================================
Coverage 100% 100%
======================================
Files 38 39 +1
Lines 784 763 -21
======================================
- Hits 784 763 -21
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Less and cleaner code with all the tests passing 🎉
Just 2 suggestions (which doesn't have to be done)
{ | ||
rules.ThrowIfNull(); | ||
|
||
var result = ValitResult.Success; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an enum
to me, maybe it would be clearer to do new ValitResult()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always hover on var
so it's clear that's IValitStrategy
object :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True 😄
|
||
if(!result.Succeeded) | ||
{ | ||
strategy.Fail(rule, result, out bool cancel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I said this a couple of days ago, but maybe lets make strategy.Fail
return a bool
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arekbal did this as an out param. I think he's the right person to ask why we actualy use it :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner API when implementing that method. Instead of guessing or going through some docs to find why method returns bool and what it means(true for cancel or continue?), one just implements a method with clear declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too 👍 the best refactoring! :) Less code and looks clean.
Info
This Pull Request is related to issue no. #150
With some hard time for figuring out how to make it better but I did it :D !
Changes
ValitRuleExtensions
class withVallidateRules()
extensionIValitRules<TObject>
CollectionValitRule
NestedObjectCollectionValitRul