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

[a-h-i]

trafficstars

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

Stripped spaces in isRgbColor function

Checklist

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

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[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?

[a-h-i]

@braaar @WikiRik is this good to go after the updated README?

[a-h-i]

@braaar I don't think the default param option would really work fiddle

[braaar]

@braaar I don't think the default param option would really work fiddle

You're right.

We should probably do it like this

[a-h-i]

@braaar is b339ed3 more like what you had in mind?

[braaar]

You should change the PR title to feat(isRgbColor): add `strict` option to allow/disallow spaces between color values

[braaar]

Honestly, maybe we should change the option name to allowSpaces instead of strict?

[a-h-i]

Honestly, maybe we should change the option name to allowSpaces instead of strict?

Yeah will probably be better and would also be more inline with how percent values are treated

[a-h-i]

@braaar updated PR name and variable name

[a-h-i]

@braaar is there a specific review process we need to go through ?

[braaar]

@braaar is there a specific review process we need to go through ?

You're gonna need an approval from a maintaner. As you can see, there is a bit of a backlog on this repo, so we need to be patient.

[a-h-i]

@profnandaa rebased and resolved merge conflicts

[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?