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

add(isURL): added validate_length option #1397

Merged
merged 5 commits into from
Aug 1, 2020
Merged

add(isURL): added validate_length option #1397

merged 5 commits into from
Aug 1, 2020

Conversation

tomgrossman
Copy link
Contributor

fix for issue: #1396

Today the isURL function validate hard-coded for max 2083 string length. This is not part of the RFC and should be able to skip this validation.

Checklist

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

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! 🎉
@tux-tn -- could you have a second look on this?

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomgrossman Thank you for your contribution 🎉

lib folder is generated by babel, you need to make your changes inside src/lib folder. In addition, you'll have to update existing tests (there is a test case where the URL is longer than 2083) and add new tests for your added option

@tomgrossman
Copy link
Contributor Author

@tux-tn fixed. for some reason I can't click on the "re-request review" button, it doesn't work...

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@profnandaa profnandaa merged commit ed86b0a into validatorjs:master Aug 1, 2020
@bruno-smaldone
Copy link

Opened related issue: #1412

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.

4 participants