-
-
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(isISBN): allow usage of options object #2157
feat(isISBN): allow usage of options object #2157
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. |
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
}); | ||
}); | ||
|
||
describe('(legacy syntax)', () => { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 to me, other than that double exclamation mark remark, which is maybe a bit on the nitpicking side :-)
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.
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 |
This PR implements steps 1 and 2 of #1874 for
isISBN
.Checklist