-
-
Notifications
You must be signed in to change notification settings - Fork 214
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Feature toBeHexadecimal Matcher #152
Add Feature toBeHexadecimal Matcher #152
Conversation
Codecov Report
@@ Coverage Diff @@
## master #152 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 101 103 +2
Lines 488 500 +12
Branches 83 85 +2
=====================================
+ Hits 488 500 +12
Continue to review full report at Codecov.
|
expect(() => expect('#eeggee').toBeHexadecimal()).toThrowErrorMatchingSnapshot(); | ||
}); | ||
|
||
test('fails when given non-string', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth having a test for a string that isn't hexadecimal as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This boundary is already covered in the predicate test.
export default expected => { | ||
const re1 = RegExp(/^#\b[a-f0-9]{6}\b/gi); | ||
const re2 = RegExp(/^#\b[a-f0-9]{3}\b/gi); | ||
return re1.test(expected) || re2.test(expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth naming these a bit more explicitly, e.g.
shortRegex
longRedex
I only ask because i read the regex 3 times before i spotted the 6 and 3 馃寽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This was lazy on my part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice one 馃憤 I can only give approval and nod quietly in the corner unfortunately though :P
LGTM thanks @mvpowers I am on my phone atm I鈥檒l release this when I鈥檓 at my computer next 馃槃 |
What
Feature
Why
This feature satisfies #149. This is a new matcher to verify a string is a valid HTML hex color.
Notes
Please let me know if I need to make any modifications 馃憤
Housekeeping
yarn contributor
)