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: add function test names to email and url #292

Merged
merged 3 commits into from
May 16, 2019

Conversation

aytee17
Copy link
Contributor

@aytee17 aytee17 commented Aug 19, 2018

I noticed that string.email() and string.url() don't have associated test function names and appear as undefined in the tests array of SchemaDescription.

This just makes sure that a proper name is sent through the matches function.

@aytee17 aytee17 closed this Aug 21, 2018
@jquense
Copy link
Owner

jquense commented Aug 21, 2018

why close this?

@aytee17
Copy link
Contributor Author

aytee17 commented Aug 21, 2018

Honestly, I thought I did something wrong (this is my first PR changing code) so I closed it out of embarrassment.

@aytee17 aytee17 reopened this Aug 21, 2018
@jquense
Copy link
Owner

jquense commented Aug 21, 2018

Nah, looks great, thanks for the PR, I'm a bit short on time, but I'll get to reviewing it more throughly!

@dontub
Copy link

dontub commented Mar 15, 2019

matches must have a name itself if none is provided, probably just "matches". Without name at least the async validation is (correctly) rejected, but without errors. Until this is fixed I have to overwrite the matches method.

src/string.js Outdated Show resolved Hide resolved
aytee17 and others added 2 commits May 16, 2019 20:33
…nction

Co-Authored-By: Jason Quense <monastic.panic@gmail.com>
@jquense jquense merged commit 7e94395 into jquense:master May 16, 2019
@jquense jquense changed the title Add function test names to email and url feat: add function test names to email and url May 16, 2019
@aytee17 aytee17 deleted the email-url-names branch May 22, 2019 10:47
@juice49
Copy link

juice49 commented Jun 13, 2019

@jquense do you have a schedule for publishing a new version of yup to npm?

I'm relying on this patch in one of my projects, so I've included the yup master branch for now. It would be great to be able to use yup from npm.

Happy to help if there is anything I can do 🙂.

Thank you for this very useful lib!

@sawyerh
Copy link

sawyerh commented Jun 21, 2019

Thanks for tackling this @aytee17, I just noticed this too and was about to open a PR. Glad to see this got merged, and eagerly awaiting its release. Our system logs the name to help track what type of errors users are running into.

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.

5 participants