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(isSlug): isSlug validator #1096

Merged
merged 1 commit into from Oct 16, 2019
Merged

Conversation

Nicanor008
Copy link
Contributor

Closes #952

@profnandaa
Copy link
Member

Really great for your first PR @Nicanor008! I'll review soon. Thanks 🎉

@Nicanor008 Nicanor008 force-pushed the master branch 2 times, most recently from 3f1b1fa to 58b9944 Compare August 27, 2019 10:09
Copy link
Member

@ezkemboi ezkemboi left a 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.

let charset = '^([a-z0-9\\-]{1,})$';
let regex_charset = new RegExp(`${charset}`);
let regex_boundaries_consecutive = /[-_]{2,}/;
let regex_boundaries_leading = /^[-_]/;
Copy link
Member

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:

  1. Use of camel casing e.g
let regexCharset = new RegExp(`${charset}`)
  1. 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
 )
  1. 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));
}

Copy link
Contributor Author

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.

validator: 'isSlug',
args: ['cs_67CZ'],
valid: ['cs-cz', 'cscz'],
invalid: ['cs--------CZ12 ', '@#_$@'],
Copy link
Member

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_,

@ezkemboi
Copy link
Member

Also, update the README file.

@Nicanor008
Copy link
Contributor Author

thanks, @ezrqnkemboi I'll work on the feedback

@Nicanor008 Nicanor008 force-pushed the master branch 2 times, most recently from 844bdcb to 425bcd7 Compare August 30, 2019 16:32
@Nicanor008
Copy link
Contributor Author

Feedback implemented @ezrqnkemboi
cc: @profnandaa

import assertString from './util/assertString';

let charsetRegex = /^[^-_](?!.*?[-_]{2,})([a-z0-9\\-]{1,}).*[^-_]$/;

Copy link
Member

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.

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, thanks for your contrib! 🎉

@profnandaa profnandaa merged commit 2222e5a into validatorjs:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IsSlug
3 participants