feat(isISBN): allow usage of options object - #2157
Conversation
Sorry, something went wrong.
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. |
Sorry, something went wrong.
profnandaa
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
| }); | ||
| }); | ||
|
|
||
| describe('(legacy syntax)', () => { |
There was a problem hiding this comment.
Thanks for the backward compatibilty! 👍
Sorry, something went wrong.
|
One more review, we can get this in too, in this coming release. /cc. @pano9000 |
Sorry, something went wrong.
| } | ||
| 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
Sorry, something went wrong.
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
Sorry, something went wrong.
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
Sorry, something went wrong.
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 :-)
Sorry, something went wrong.
pano9000
left a comment
There was a problem hiding this comment.
thank you, looks good to me
Sorry, something went wrong.
|
@pano9000 -- wondering why your review is not being counted as a member's review 🤔 (still showing grey tick). |
Sorry, something went wrong.
That's only happening when something that has write access does the review, and I don't think they have that |
Sorry, something went wrong.
|
@profnandaa @WikiRik is correct, I only am a "mod member" currently (which is totally fine for now), so my review do not "fully" count |
Sorry, something went wrong.
This PR implements steps 1 and 2 of #1874 for isISBN.
Checklist