Feature/114 comparing two properties #131

paw3lx · 2017-12-02T18:48:49Z

Info

This Pull Request is related to issue no. #114

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

…Provider into Valitator

Issue 117: Altered ValitRuleError to support both a static string and a Func<string> - Updated ExtensionMethods to support both - Added tests to the new feature

Added Func as a suffix for the Func<string>

Set as string.Empty instead of null

…ency

codecov · 2017-12-02T18:51:33Z

Codecov Report

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

@@          Coverage Diff           @@
##           develop   #131   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           38     38           
  Lines          784    815   +31     
======================================
+ Hits           784    815   +31

Impacted Files	Coverage Δ
src/Valit/ValitRuleUInt64Extensions.cs	`100% <ø> (ø)`	⬆️
src/Valit/ValitStrategies.cs	`100% <ø> (ø)`	⬆️
src/Valit/ValitException.cs	`100% <ø> (ø)`	⬆️
src/Valit/ValitRuleUInt32Extensions.cs	`100% <ø> (ø)`	⬆️
src/Valit/ValitRuleInt32Extensions.cs	`100% <ø> (ø)`	⬆️
src/Valit/CompleteValitStrategy.cs	`100% <ø> (ø)`	⬆️
src/Valit/MessageProvider/EmptyMessageProvider.cs	`100% <ø> (ø)`	⬆️
src/Valit/Exceptions/ValitExceptionExtensions.cs	`100% <ø> (ø)`	⬆️
src/Valit/ValitRuleDoubleExtensions.cs	`100% <ø> (ø)`	⬆️
src/Valit/ValitStrategiesExtensions.cs	`100% <ø> (ø)`	⬆️
... and 31 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 eb68fc0...1eb3bfb. Read the comment docs.

GooRiOn · 2017-12-02T21:04:49Z

src/Valit/ValitRules.cs

+            selector.ThrowIfNull();
+            ruleFunc.ThrowIfNull();
+
+            var lastEnsureRule = new ValitRule<TObject, TProperty>(selector, _messageProvider, ruleFunc);


You should call AddEnsureRulesAccessors here to get all rules from the chain. BTW this name is stupid :D We should change that to AddEnsureRules instead.

I can't call AddEnsureRuleAccessors because it has different arguments. I did few changes to get all rules from the chain.
I cannot call ruleFunc() to create ValitRule, because we need @object to do this. So basically we can do this when Valit method is called.

sorry, my bad :D

GooRiOn · 2017-12-02T21:07:21Z

I like this way of creating the rules. Now we need to decide what to do with that.

@arekbal @dbarwikowski @tdeschryver what are your thoughts?

We should not hurry with the decision, tommorow I'd like to release version 0.2.0 so this task will be scheduled for the 0.3.0 release (so before christmass).

…eature/114

timdeschryver

Yep, I like the syntax which is introduced in this PR.
Just a couple remarks, @GooRiOn thoughts?

timdeschryver · 2017-12-04T13:03:18Z

src/Valit/IValitRules.cs

+         IValitRules<TObject> EnsureFor<TProperty>(Func<TObject, IEnumerable<TProperty>> selector, Func<IValitRule<TObject, TProperty>, IValitRule<TObject, TProperty>> ruleFunc);
+         IValitRules<TObject> EnsureFor<TProperty>(Func<TObject, IEnumerable<TProperty>> selector, IValitRulesProvider<TProperty> valitRulesProvider) where TProperty : class;
+         IValitRules<TObject> For(TObject @object);
+         IEnumerable<IValitRule<TObject>> GetAllRules();         


Here are some extra whitespaces at the end of the line. (also an extra space for every property in this interface)
I don't know why, but I thought the .editorconfig would fix these.

Can I ask which editor you're using? 😄

Visual Studio Code on macOS. I will take a look on this.

I forgot this, but it seems I have this extension installed...

timdeschryver · 2017-12-04T13:03:31Z

src/Valit/IValitRules.cs

+         IValitRules<TObject> For(TObject @object);
+         IEnumerable<IValitRule<TObject>> GetAllRules();         
+         IEnumerable<IValitRule<TObject>> GetTaggedRules();
+         IEnumerable<IValitRule<TObject>> GetUntaggedRules();         


Same as above.

timdeschryver · 2017-12-04T13:08:29Z

src/Valit/Rules/CollectionValitRule.cs

+
+        public IEnumerable<IValitRule<TObject>> GetEnsureRules(TObject @object)
+        {
+            return new List<IValitRule<TObject>>{ this };


No need to initialize a List here, we can do new []{ this };.
Maybe also use a Expression-bodied member, since we're using it everywhere else?

timdeschryver · 2017-12-04T13:10:28Z

src/Valit/Rules/NestedObjectCollectionValitRule.cs

@@ -52,5 +52,9 @@ IValitResult IValitRule<TObject>.Validate(TObject @object)
            return result;
        }

+        public IEnumerable<IValitRule<TObject>> GetEnsureRules(TObject @object)
+        {
+            return new List<IValitRule<TObject>>{ this };


Same as above.

timdeschryver · 2017-12-04T13:10:40Z

src/Valit/Rules/NestedObjectValitRule.cs

+
+        public IEnumerable<IValitRule<TObject>> GetEnsureRules(TObject @object)
+        {
+            return new List<IValitRule<TObject>>{ this };


Same as above.

timdeschryver · 2017-12-04T13:15:50Z

src/Valit/Rules/ValitRule.cs

+        IEnumerable<IValitRule<TObject>> IValitRule<TObject>.GetEnsureRules(TObject @object)
+        {
+            @object.ThrowIfNull();
+            if (_ruleFunc != null)


It doens't change anything, but I think in the other places we're using an if we're surrounding it with braces.
Personally I would also check if _ruleFunc is null instead of not null, contradictory ifs are harder to read imo, @GooRiOn thoughts?

timdeschryver · 2017-12-04T13:16:33Z

src/Valit/Rules/ValitRule.cs

+            if (_ruleFunc != null)
+                return _ruleFunc(this, @object).GetAllEnsureRules();
+
+            return new List<IValitRule<TObject>>{ this };


Same as above, no need to initialize a List.

timdeschryver · 2017-12-04T13:21:15Z

tests/Valit.Tests/String/String_IsEqualTo_Tests.cs

+        {
+            IValitResult result = ValitRules<Model>
+                .Create()
+                .Ensure(s => s.Value, (_, m) => _.IsEqualTo(m.Value).MinLength(3))


No need to check .MinLength(3) here.

timdeschryver · 2017-12-04T13:21:38Z

src/Valit/Rules/CollectionValitRule.cs

-                .For(@object)
-                .Validate();
+		private IValitResult ValidatePropertyRules(IEnumerable<IValitRule<TObject>> propertyRules, TObject @object)
+			=> ValitRules<TObject>


Same as above, these are tabs instead of spaces.
@GooRiOn Are you OK with checking these 'problems' during a PR?

GooRiOn and others added 30 commits November 20, 2017 10:53

Removed unused strategy in ValitRule

fd85f84

Added support for nested objects

24c0f76

removed message provider from nested object valit rule

cf19391

Added internal Valitator, added extension for transforming ValitRules…

0ce93d4

…Provider into Valitator

Added unit tests for nested objects and valitator

3593c10

Made Tags in NestedObjectValitRul explicit

9c40449

fix typo

89961d8

arrange test similar like the other tests

ad29474

add boolean support

07b3189

add required

9824a9d

HasValue checks for nullables

47ef7e4

fix indention similar to other files

64436cf

Issue 117: Added support for variable messages

7dce801

Issue 117: Altered ValitRuleError to support both a static string and a Func<string> - Updated ExtensionMethods to support both - Added tests to the new feature

Minor stylechange

c735467

Added Func as a suffix for the Func<string>

Replaced default value for function to be empty string

493ee61

Set as string.Empty instead of null

remove spaces in gitignore

1aa2a6a

add reference to watcher tools

6de4de4

document dotnet watch test

2d6ab7b

add gitattributes

7189ec3

Added missing Required method for string type

4116619

Revert 4116619

5d0059e

Added missing Required method on string type.

b8e61cc

PR fixes

c52618f

Changed NullValue to EmptyValue in Empty string tests for Required

7c4c23a

Added first version of collection valit rule

b3660c8

Added support for collections

c6cb9ae

Fixed typos

a8453d1

Changed NestedObjectValitRule so it uses Valitator inside

8c8b73d

Made Tags public in all IValitRule implementations just for a consist…

421eeeb

…ency

PR fixes

c089d1c

added possibility to compare two properties

c36adfd

GooRiOn reviewed Dec 2, 2017

View reviewed changes

timdeschryver and others added 5 commits December 2, 2017 22:08

add editorconfig

e0dd752

add default format conventions

6a5a4f7

autofix files with eclint

395ce14

call GetAllEnsureRules() before validation

148679b

Merge branch 'develop' of https://github.com/valit-stack/Valit into f…

856f4a6

…eature/114

timdeschryver reviewed Dec 4, 2017

View reviewed changes

paw3lx added 2 commits December 4, 2017 15:24

removed whitespaces, added expression-bodied members

898ff25

removed unnecessary checking in test

1eb3bfb

GooRiOn force-pushed the develop branch from 445f3da to eb68fc0 Compare December 5, 2017 09:18

paw3lx closed this Dec 5, 2017

paw3lx deleted the feature/114 branch December 5, 2017 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/114 comparing two properties #131

Feature/114 comparing two properties #131

paw3lx commented Dec 2, 2017

codecov bot commented Dec 2, 2017 •

edited

Loading

GooRiOn Dec 2, 2017

paw3lx Dec 3, 2017 •

edited

Loading

GooRiOn Dec 3, 2017

GooRiOn commented Dec 2, 2017

timdeschryver left a comment

timdeschryver Dec 4, 2017

paw3lx Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

timdeschryver Dec 4, 2017

Feature/114 comparing two properties #131

Feature/114 comparing two properties #131

Conversation

paw3lx commented Dec 2, 2017

Info

Changes

codecov bot commented Dec 2, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

paw3lx Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooRiOn commented Dec 2, 2017

timdeschryver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 2, 2017 •

edited

Loading

paw3lx Dec 3, 2017 •

edited

Loading