-
-
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(isSlug): isSlug validator #1096
Conversation
Really great for your first PR @Nicanor008! I'll review soon. Thanks 🎉 |
3f1b1fa
to
58b9944
Compare
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 have requested some slight changes and gave comments and some appropriate solutions on the same.
src/lib/isSlug.js
Outdated
let charset = '^([a-z0-9\\-]{1,})$'; | ||
let regex_charset = new RegExp(`${charset}`); | ||
let regex_boundaries_consecutive = /[-_]{2,}/; | ||
let regex_boundaries_leading = /^[-_]/; |
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.
Good work on this. Here are my comments and some changes request:
- Use of camel casing e.g
let regexCharset = new RegExp(`${charset}`)
- We can make some changes instead of using
new Regex
example provided
let charsetRegex = /^([a-z0-9\\-]{1,})$/;
// remove let regex_charset = new RegExp(`${charset}`);
// Other code remains the same
return (
charsetRegex.test(str) &&
// Other code here
)
- Also, we can make the regex further combined
import assertString from './util/assertString';
let charsetRegex = /^[^-_](?!.*?[-_]{2,})([a-z0-9\\-]{1,}).*[^-_]$/;
export default function isSlug(str) {
assertString(str);
return (charsetRegex.test(str));
}
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 this Feedback @ezrqnkemboi, makes good use of the re-usable variable.
test/validators.js
Outdated
validator: 'isSlug', | ||
args: ['cs_67CZ'], | ||
valid: ['cs-cz', 'cscz'], | ||
invalid: ['cs--------CZ12 ', '@#_$@'], |
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.
Add other invalid cases such as:
-not-slug,
not-slug-,
_not-slug,
not-slug_,
Also, update the README file. |
thanks, @ezrqnkemboi I'll work on the feedback |
844bdcb
to
425bcd7
Compare
Feedback implemented @ezrqnkemboi |
import assertString from './util/assertString'; | ||
|
||
let charsetRegex = /^[^-_](?!.*?[-_]{2,})([a-z0-9\\-]{1,}).*[^-_]$/; | ||
|
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.
Changes I requested were implemented here.
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, thanks for your contrib! 🎉
Closes #952