Skip to content
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

String MaxLength rule should accept empty string #166

Closed
dirk-vdb opened this issue Aug 15, 2018 · 3 comments
Closed

String MaxLength rule should accept empty string #166

dirk-vdb opened this issue Aug 15, 2018 · 3 comments

Comments

@dirk-vdb
Copy link

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
Copy link
Contributor

bovorasr commented Oct 3, 2018

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
Copy link
Contributor

bovorasr commented Oct 3, 2018

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
Copy link
Member

GooRiOn commented Oct 7, 2018

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 :)

@GooRiOn GooRiOn closed this as completed Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants