feat(isRgbColor): add allowSpaces option to allow/disallow spaces between color values

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?

[a-h-i]

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes#2028

Stripped spaces in isRgbColor function

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2029 +/- ##
==========================================
Coverage 100.00% 100.00% ==========================================
Files 108 104 -4 Lines 2482 2215 -267 Branches 627 481 -146 ==========================================
- Hits 2482 2215 -267 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braaar]

I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

	'rgba(255, 255, 255, 0.5)',
	],
	'rgba(255, 255, 255, 0.5)',
	'r g b( 0, 251, 222 )',
	],

[a-h-i]

changed so that it does not ignore spaces between rgb at start, and added the test cases

[WikiRik]

Why did you add this? Isn't it already checked in the first section of the other regexes?

[WikiRik]

Ah, I see. It's because the newly added test is invalid. Why is that?

[a-h-i]

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

[a-h-i]

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

[braaar]

LGTM!

[braaar]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

[a-h-i]

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

[braaar]

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

[WikiRik]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

[braaar]

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

[WikiRik]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

[a-h-i]

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

[braaar]

I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

[WikiRik]

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

	if(typeof(options)==='object'){
	min=options.min||0;
	max=options.max;
	}else{// backwards compatibility: isLength(str, min [, max])
	min=arguments[1]||0;
	max=arguments[2];
	}

[a-h-i]

is something like 301e707 what you had in mind? 🙂

[braaar]

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

[a-h-i]

@braaar updated readme

[WikiRik]

I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

[braaar]

I agree, we should have the object as the default behaviour. That makes the most sense to me.

[braaar]

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

[a-h-i]

@braaar changed in 9d0d3b3

[braaar]

I think this is a clearer explanation

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[profnandaa]

LGTM, thanks for your contrib! 🎉

[a-h-i]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

[WikiRik]

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

[profnandaa]

@rubiin -- can take a look at this again? looks like it was good to go?