-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(isBase32): Add option for Crockford's base 32 alternative #1888
Conversation
…to isBase32 method
Codecov Report
@@ Coverage Diff @@
## master #1888 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 102 102
Lines 2072 2078 +6
Branches 472 473 +1
=========================================
+ Hits 2072 2078 +6
Continue to review full report at Codecov.
|
test/validators.js
Outdated
|
||
test({ | ||
validator: 'isBase32', | ||
args: [{ crockford: true }], | ||
valid: [ | ||
'91JPRV3F41BPYWKCCGGG', | ||
'60', | ||
'64', | ||
'B5QQA833C5Q20S3F41MQ8', | ||
], | ||
invalid: [ | ||
'91JPRV3F41BUPYWKCCGGG', | ||
'B5QQA833C5Q20S3F41MQ8L', | ||
'60I', | ||
'B5QQA833OULIC5Q20S3F41MQ8', | ||
], | ||
}); |
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.
I would suggest you move this test out to its own it()
statement so it is more clear precisely what is being tested. At least many other tests are split up like this. See 'should validate JSON'
and 'should validate JSON with primitives'
.
Examples to the contrary are found as well ('should validate ISSNs'
has everything in one it()
statement), so I suppose a clarification of the convention here would be nice, but that's a bigger topic.
I move the test out. 🙂 |
Co-authored-by: Brage <brage.aarset@gmail.com>
I think the title should be feat(isBase32): Add option for Crockford's base 32 alternative |
I updated the name of the PR :) |
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.
LGTM.
// Sorry for the delay in reviewing this.
This PR adds feature requested on issue #1857
Checklist