feat(isISBN): allow usage of options object#2157
feat(isISBN): allow usage of options object#2157profnandaa merged 2 commits intovalidatorjs:masterfrom WikiRik:isISBN-options-refactor
Conversation
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #2157 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 105 105
Lines 2334 2334
Branches 586 586
=========================================
Hits 2334 2334
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| }); | ||
| }); | ||
|
|
||
| describe('(legacy syntax)', () => { |
There was a problem hiding this comment.
Thanks for the backward compatibilty! 👍
|
One more review, we can get this in too, in this coming release. /cc. @pano9000 |
| } | ||
| const sanitized = str.replace(/[\s-]+/g, ''); | ||
|
|
||
| const sanitizedIsbn = isbn.replace(/[\s-]+/g, ''); |
There was a problem hiding this comment.
a bit off topic for the PR;
I would like to see that "sanitization" step replaced with as option, maybe something like a "strict" mode, checking/ignoring for spaces or hyphens only inside the ISBN, not also at the beginning or end of the ISBN string.
but that shouldn't be part of this PR, just noting it down for the future
There was a problem hiding this comment.
Yes, I had that in mind as well when working on this but from personal experience people write the ISBN down differently. I haven't checked if there are rules on where the - should be, but I think that in this case sanitising whitespaces and - adds value to this specific validator.
About the beginning/end of the string; that's not something I thought of but if you want I can add that
There was a problem hiding this comment.
no, don't worry about it here, IMHO that would be out of scope for this particular PR - let's keep it simple and get this merged and then after the release, we can work on that via a new PR, I'd say
pano9000
left a comment
pano9000
left a comment
There was a problem hiding this comment.
looks good to me, other than that double exclamation mark remark, which is maybe a bit on the nitpicking side :-)
pano9000
left a comment
pano9000
left a comment
There was a problem hiding this comment.
thank you, looks good to me
|
@pano9000 -- wondering why your review is not being counted as a member's review 🤔 (still showing grey tick). |
That's only happening when something that has write access does the review, and I don't think they have that |
|
@profnandaa @WikiRik is correct, I only am a "mod member" currently (which is totally fine for now), so my review do not "fully" count |
3 participants
This PR implements steps 1 and 2 of #1874 for
isISBN.Checklist