valit-stack / Valit

Valit is dead simple validation for .NET Core. No more if-statements all around your code. Write nice and clean fluent validators instead!
MIT License
322 stars 26 forks source link

Added support for default messages #155

Closed GooRiOn closed 6 years ago

GooRiOn commented 6 years ago

Info

This Pull Request is related to issue no. #113

Since default messages are required in #148 issue I decided to implement that one. There're lots o changes but basically most of them is just adding new, internal WithDefaultMessage method. To get the proper messages I changed all property selectors to expressions so I could fairly easy take the name of the property and put into message. The next step was to implement WithDefaultMessage method which do the trick - creates the message and puts it into the rule. Another change is placed inside ValitRuleError class. I added new property IsDefault which informs whether the error comes from default error message or not. This is used inside the Validate method in ValitRule when I check whether errors collection contains any non-default messages. If so, we take only non-default. Otherwise we get default. The last change was changing the implementation of Email rule since it used Match beneath which caused doubled default messages.

However this feature have two limitations:

  1. Property selector must be a MemberExpression. If it's not then instead of property name we put string.Empty. To explain that I'll use an example. This is what we support:

Ensure(m => m.Value)

It's very simple to get the "Value" name from the tree. However in some of our tests we did something like this:

Ensure(m => useNullValue? m.NullValue : m.NullableValue)

Now this one is a ConditionalExpression which we need to evaluate first in order to get the proper expression (proper branch in the expression tree). Of course we could do this since we have an access to the useNullValue. But consider the following example:

Ensure(m => m.UseNullableValue ? m.NullableValue : m.NullValue)

It's another ConditionalExpression but we cannot evaluate it without the actual object which is passed later in the For method. Because of that, together with Arek, we decided that for safetyness we'll support only simple selectors as shown at the beggining.

  1. When we do EnsureFor the only name we have is the name of the collection, not each element. So in this kind of messages the name of property is for a now "p" :D I guess I could replace this with "element".

Changes

codecov[bot] commented 6 years ago

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #155   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           39     40    +1     
  Lines          763    808   +45     
======================================
+ Hits           763    808   +45
Impacted Files Coverage Δ
src/Valit/ValitRules.cs 100% <ø> (ø) :arrow_up:
src/Valit/Errors/ValitRuleError.cs 100% <100%> (ø) :arrow_up:
src/Valit/ValitRulePropertyExtensions.cs 100% <100%> (ø) :arrow_up:
src/Valit/ValitRuleEnumerableExtensions.cs 100% <100%> (ø) :arrow_up:
src/Valit/ValitRuleDateTimeOffsetExtensions.cs 100% <100%> (ø) :arrow_up:
src/Valit/ValitRuleStringExtensions.cs 100% <100%> (ø) :arrow_up:
src/Valit/ValitRuleInt16Extensions.cs 100% <100%> (ø) :arrow_up:
src/Valit/ValitRuleInt32Extensions.cs 100% <100%> (ø) :arrow_up:
src/Valit/Rules/ValitRule.cs 100% <100%> (ø) :arrow_up:
src/Valit/Rules/NestedObjectCollectionValitRule.cs 100% <100%> (ø) :arrow_up:
... and 17 more

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 04d2b49...06d6eff. Read the comment docs.

timdeschryver commented 6 years ago

As said in #113 I also had a WIP, but I wasn't quite happy with the result yet.

The main differences are:

For the rest I think its more or less the same, I came to the same conclusion as you did for the Ensure(m => useNullValue? m.NullValue : m.NullableValue) case.

GooRiOn commented 6 years ago

@tdeschryver I see your point. Well, to me this kind of message templates I did, could be still pretty easily customised with all things from #146. When it comes to default message apporach - my intention was not to extend the ValitRule class even more but to extract this whole process to some extension method. Of course this caused changing all rules but I'd say it's the way it should be implemented for one reason - it looks what it does. There's no "hidden" functionality inside Validate method or somewhere else. It's just:

=> Rule().WithDefaultMessage()

so it's easy to get what it does and where it does. This is crucial when we think about folks who would like to read the code and change it in the future.

timdeschryver commented 6 years ago

@GooRiOn all true 😄