Feature/114 comparing two properties

[paw3lx]

Info

This Pull Request is related to issue no. #114

After the latest problems with develop branch I talked to @GooRiOn. We decided that the best will be to create new PR on this repo instead of my fork.

I will just copy/paste notes from previous PR:
Guys, I added few lines of code to provide possibility to compare two properties. It's just a draft, so please take a look. I was not sure about it so I just added one Ensure method and one test to test if it's working.
I think that now we have two possibilities:

• To have many Ensure methods. They can look like in this PR

• Replace Ensure methods to have only one way of creating ValitRules. This could be big change I guess. As @GooRiOn said, it could piss off all folks that use Valit.
  What do you think?

Changes

• Added Ensure method to provide functionality of comparing two properties

[codecov]

Codecov Report

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

@@          Coverage Diff           @@
##           develop   #143   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           39     39           
  Lines          763    805   +42     
======================================
+ Hits           763    805   +42

Impacted Files	Coverage Δ
src/Valit/Rules/NestedObjectCollectionValitRule.cs	100% <ø> (ø)	⬆️
src/Valit/Rules/ValitRuleExtensions.cs	100% <100%> (ø)	⬆️
src/Valit/Rules/ValitRule.cs	100% <100%> (ø)	⬆️
src/Valit/Rules/ValitRuleAccessorExtensions.cs	100% <100%> (ø)	⬆️
src/Valit/ValitRules.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 04d2b49...2385d28. Read the comment docs.

[GooRiOn]

Honestly, I don't get the part related to the GetEnsureRules :/ Could you explain it a little bit? According to your question whether we should stick to one/many Ensure, after a while I'd go for many overloads. Let folks decide which selector is fine for which validation.

[paw3lx]

Ok, I will try to explain this.
So basically our goal is to get all rules from the chain passed in Ensure function. To do this we have to call GetAllEnsureRules on the result from ruleFunc delegate (Func<IValitRule<TObject, TProperty>, TObject, IValitRule<TObject, TProperty>>). So now we need call ruleFunc and to do this we need an instance of TObject. This is done inside GetEnsureRules function: _ruleFunc(this, @object).GetAllEnsureRules();

We cannot do this inside Ensure function because we don't have TObject instance. So we need to do this later, when Validate function is called.

I hope that you understand my explanation. As I said at the beginning this is just an idea. Maybe you will come up with better solution :)

[GooRiOn]

Now I get this :D So to things I noticed:

• In your Ensure() overload you can get rid of GetAllEnsureRules() since it always returns 1 item
• I'd move EnsureRules to IValitRuleAccessor since it should not be exposed to the users

I also think that the Validate() and ValidateRule are now too complicated right now, but unfortunetly, I have no idea how to simplify this. Any suggestion @arekbal @tdeschryver ?

[timdeschryver]

@GooRiOn I'm a bit busy atm.
I'll get back to this in the weekend/next week.

[paw3lx]

In your Ensure() overload you can get rid of GetAllEnsureRules() since it always returns 1 item

Done

I'd move EnsureRules to IValitRuleAccessor since it should not be exposed to the users

Done as well. To do this I had to create new IValitRuleAccessor. Please check if everything is ok.

[timdeschryver]

@GooRiOn the methods seems fine to me.
Maybe the only thing would be to change _strategy.Fail so it returns a bool instead of using an out parameter for it?

[timdeschryver]

Not something that is changed in this PR, but why is the .ToList() needed here?

[GooRiOn]

to avoid multiple enumerations

[GooRiOn]

@tdeschryver agree with your statements but I think we should discuss how to simplify this code. I do what I does but it quite complicated and duplicates the code responsible for handling validation. As I mentioned before, the issue is that I can't come up with any better idea now. It just looks to me that we could do this better (and this applies also to my valit rules responsible for handling nested objects and collection where this code is also duplicated).

[timdeschryver]

Gotya!

[GooRiOn]

Created an issue for the duplication #150

[paw3lx]

I removed the out argument from ValidateRule. It looks a little bit simpler. What do you think guys? Do you have any idea what else we could simplify?

[GooRiOn]

@paw3lx could we do some Skype/Hangout call, so you could explain the whole flow to me? I guess I know what happens here, but would be great to confirm that during the call. It'd be way simpler for me to understand the whole thing. Also, I could explain my PR to you as well :)

[paw3lx]

@GooRiOn Yes, sure. Just contact me and we can do some Skype call :) I think you have my email or snapchat.

[GooRiOn]

This PR is not resolved for too long. Need to close it :(