Skip to content
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

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

WikiRik
Copy link
Member

@WikiRik WikiRik commented Jan 29, 2023

This PR implements steps 1 and 2 of #1874 for isISBN.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (845148d) compared to base (753c29d).
Patch coverage: 100.00% of modified lines in pull request are covered.

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           
Impacted Files Coverage Δ
src/lib/isISBN.js 100.00% <100.00%> (ø)

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

profnandaa
profnandaa previously approved these changes Jan 30, 2023
Copy link
Member

@profnandaa profnandaa left a 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)', () => {
Copy link
Member

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! 👍

@profnandaa
Copy link
Member

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, '');
Copy link
Contributor

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

Copy link
Member Author

@WikiRik WikiRik Jan 30, 2023

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

Copy link
Contributor

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

Copy link
Contributor

@pano9000 pano9000 left a 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 :-)

src/lib/isISBN.js Outdated Show resolved Hide resolved
Copy link
Contributor

@pano9000 pano9000 left a 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

@profnandaa
Copy link
Member

@pano9000 -- wondering why your review is not being counted as a member's review 🤔 (still showing grey tick).

@profnandaa profnandaa merged commit 6dba289 into validatorjs:master Jan 30, 2023
@WikiRik
Copy link
Member Author

WikiRik commented Jan 30, 2023

@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

@pano9000
Copy link
Contributor

@profnandaa @WikiRik is correct, I only am a "mod member" currently (which is totally fine for now), so my review do not "fully" count

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants