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

String MaxLength rule should accept empty string #166

Closed dirk-vdb closed 6 years ago

dirk-vdb commented 6 years ago

When we use the MaxLength rule for validating an empty string, the rule returns a validation error. This rule must check the max length of the string, not whether it is empty. Emptyness should be checked by the MinLength rule.

bovorasr commented 6 years ago

Mind if I knock some of these issues out for Hacktoberfest?

What do you think this should do for null values? Just jumping into this project, it looks like perhaps it should still throw a validation error when the string is null (?).

I'm basing that on the implementation of IsEqualTo, which states that null != null (two null values in IsEqualTo throws a validation error).

I can also see the argument for treating null like the empty string, but If MaxLength should treat a null value as effectively having a zero length, I'd be inclined to think that IsEqualTo should also treat null as being equal to null.

bovorasr commented 6 years ago

The more I look at this, the more I think Required on string should not use IsNullOrEmpty, but instead should only check null. That's consistent with the implementation of of the value type extensions + an empty string is a valid string of zero length.

The implementation of the rest of the string extensions should then also only use a null check.

To illustrate why: as it is right now, A MinLength(0) rule cannot be satisfied, and this seems like a perfectly valid validation statement. Additionally, the value type extensions seem to define Required as meaning null only (in the nullable versions).

GooRiOn commented 6 years ago

Sorry for beeing so late to the party guys. From now on I'm gonna be more involved in Valit again.

I must say that I agree pretty much with all your statements. Using string.IsNullOrEmpty was wrong choice here. However I'm not quite sure what to do with this issue because changing these all string conditions to null-checks would mean quite serious change :/

@bovorasr I;ll look at your PR and decide what to do. Anyway thank you for beeing involved in this project :)