Add alphaspace validator #1343

takaaa220 · 2024-12-07T21:40:45Z

Fixes Or Enhances

Add alphaspace validator for #1191.

Make sure that you've checked the boxes below before you submit PR:

Tests exist or have been written that cover this particular change.

@go-playground/validator-maintainers

coveralls · 2024-12-07T21:42:26Z

coverage: 74.322% (+0.004%) from 74.318%
when pulling da6de87 on takaaa220:add_alpha_space_validator
into 6c3307e on go-playground:master.

takaaa220 · 2024-12-07T21:46:41Z

regexes.go

@@ -7,6 +7,7 @@ import (

 const (
 	alphaRegexString                 = "^[a-zA-Z]+$"
+	alphaSpaceRegexString            = "^[a-zA-Z ]+$"


Would it be better for the regex to not match if the input contains only spaces?

I would leave it like that

But people already asked for what you suggest, so I don't know

#1191 (comment)

But then, should leading and trailing space are valid?

foo, trailing space, valid or invalid?

foo , leading space, valid or invalid

`` empty, invalid?

only space, invalid?

foo bar

I'm asking because the regexp will need to be adapted

Actually, I reviewed #1191 (comment) before asking the question. My question was more for clarification to ensure we’re aligned on the intended behavior.
I think that making the regex more complex to handle edge cases like these is unnecessary. Current implementation avoids unnecessary complexity while meeting the expected use cases.

I think the validator should do what it says and nothing else. If a string contains any of the alpha characters or an empty space it should be valid.

Anything about spaces before or after should then be dealt with by the user IMO. Unless someone can give a great example why " a" is invalid and "a a" should be valid I think this is good.

ccoVeille · 2024-12-08T10:24:24Z

doc.go

@@ -762,6 +762,12 @@ This validates that a string value contains ASCII alpha characters only

 	Usage: alpha

+# Alpha Space
+
+This validate that a string value contains ASCII alpha characters and spaces only


Suggested change

This validate that a string value contains ASCII alpha characters and spaces only

This validates that a string value contains ASCII alpha characters and spaces only

fixed it
da6de87

ccoVeille · 2024-12-08T10:26:11Z

regexes.go

@@ -7,6 +7,7 @@ import (

 const (
 	alphaRegexString                 = "^[a-zA-Z]+$"
+	alphaSpaceRegexString            = "^[a-zA-Z ]+$"


I would leave it like that

But people already asked for what you suggest, so I don't know

#1191 (comment)

But then, should leading and trailing space are valid?

foo, trailing space, valid or invalid?

foo , leading space, valid or invalid

`` empty, invalid?

only space, invalid?

foo bar

I'm asking because the regexp will need to be adapted

ccoVeille · 2024-12-08T10:30:02Z

README.md

@@ -134,6 +134,7 @@ validate := validator.New(validator.WithRequiredStructEnabled())
 | Tag | Description |
 | - | - |
 | alpha | Alpha Only |
+| alphaspace | Alpha Space |


I'm a bit annoyed, this could lead to obesity

I mean, someone will then ask for alphanumspace, alphaunicodespace, and so on

The regexps are computed by the lib even if no one use it, so lib becomes slower and slower.

The PR addresses the issue raised. So it's OK.

But I would like to step out a bit.

The regexp rule idea was already declined, for good reasons

Regex support #1239

So I think the way to consider doing this is via the custom validator

#1191 (comment)

@ccoVeille
Thank you for your detailed feedback. I understand and agree your concerns regarding the potential growth and its impact on library performance.

If you feel a custom validator is the better path for this functionality, I’m happy to close this pull request if necessary.

I'm a random Gopher,user of the lib

Let's wait for a maintainer's feedback

I'm a random Gopher,user of the lib

oh, I mistakenly thought you were the maintainer.

Well, the regex's are lazy loaded, so if anyone doesn't use them, it doesn't affect them as much.

In this case if people think this is a good addition, I don't think we should block it because we worry about it not being used and slightly bigger binary.

Thanks for your explanation

zemzale

LGTM

zemzale · 2024-12-10T10:55:14Z

regexes.go

@@ -7,6 +7,7 @@ import (

 const (
 	alphaRegexString                 = "^[a-zA-Z]+$"
+	alphaSpaceRegexString            = "^[a-zA-Z ]+$"


I think the validator should do what it says and nothing else. If a string contains any of the alpha characters or an empty space it should be valid.

Anything about spaces before or after should then be dealt with by the user IMO. Unless someone can give a great example why " a" is invalid and "a a" should be valid I think this is good.

zemzale · 2024-12-10T11:03:03Z

README.md

@@ -134,6 +134,7 @@ validate := validator.New(validator.WithRequiredStructEnabled())
 | Tag | Description |
 | - | - |
 | alpha | Alpha Only |
+| alphaspace | Alpha Space |


Well, the regex's are lazy loaded, so if anyone doesn't use them, it doesn't affect them as much.

In this case if people think this is a good addition, I don't think we should block it because we worry about it not being used and slightly bigger binary.

add alphaspace validator

cf1b127

takaaa220 commented Dec 7, 2024

View reviewed changes

takaaa220 marked this pull request as ready for review December 7, 2024 21:47

takaaa220 requested a review from a team as a code owner December 7, 2024 21:47

ccoVeille reviewed Dec 8, 2024

View reviewed changes

Fix doc for alpha space

da6de87

takaaa220 requested a review from ccoVeille December 9, 2024 03:44

zemzale approved these changes Dec 10, 2024

View reviewed changes

ccoVeille approved these changes Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add alphaspace validator #1343

Add alphaspace validator #1343

takaaa220 commented Dec 7, 2024

coveralls commented Dec 7, 2024 •

edited

Loading

takaaa220 Dec 7, 2024

ccoVeille Dec 8, 2024

takaaa220 Dec 9, 2024 •

edited

Loading

zemzale Dec 10, 2024

ccoVeille Dec 8, 2024

takaaa220 Dec 9, 2024 •

edited

Loading

ccoVeille Dec 8, 2024

ccoVeille Dec 8, 2024

takaaa220 Dec 9, 2024

ccoVeille Dec 9, 2024

takaaa220 Dec 9, 2024

zemzale Dec 10, 2024

ccoVeille Dec 10, 2024

zemzale left a comment

zemzale Dec 10, 2024

zemzale Dec 10, 2024

	This validate that a string value contains ASCII alpha characters and spaces only
	This validates that a string value contains ASCII alpha characters and spaces only

Add alphaspace validator #1343

Are you sure you want to change the base?

Add alphaspace validator #1343

Conversation

takaaa220 commented Dec 7, 2024

Fixes Or Enhances

coveralls commented Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takaaa220 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takaaa220 Dec 9, 2024 • edited Loading

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

zemzale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 7, 2024 •

edited

Loading

takaaa220 Dec 9, 2024 •

edited

Loading

takaaa220 Dec 9, 2024 •

edited

Loading