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: exposes context on mixed.test function and add originalValue to context #1021

Merged
merged 4 commits into from
Oct 16, 2020

Conversation

abnersajr
Copy link
Contributor

@abnersajr abnersajr commented Aug 27, 2020

This PR fixes two issues: #207 and #1007.
The discussions suggest having originalValue exposed in some way. Exposing as the second argument was one of the suggestions.
Following the API and suggestion from @jquense, would be better to have this originalValue into the this context.

In previous versions, you can't use arrow functions too, because it's being used .call(ctx...).
With these changes, you can use both ways. The this context is now exposed too as a second argument.
Eg: mixed.test('name', 'test name', (value, ctx) => ctx.originalValue)

Also edited Type Definitios: DefinitelyTyped/DefinitelyTyped#47069

README.md Outdated
@@ -706,26 +706,27 @@ use the alternate signature to provide more options (see below):
let jimmySchema = string().test(
'is-jimmy',
'${path} is not Jimmy',
value => value === 'jimmy',
value, context => value === 'jimmy',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
value, context => value === 'jimmy',
(value, context) => value === 'jimmy',

@jquense
Copy link
Owner

jquense commented Aug 27, 2020

Hi @abnersajr thanks for the PR. It looks like this PR updates the docs but doesn't implement the feature. Is that intentional or did a commit go missing somewhere?

@abnersajr
Copy link
Contributor Author

Sorry @jquense. Definitely something was misssed in my commits. I'm gonna check it ASAP. and thanks for the quick review.

@alexandernst
Copy link

What happened to this?

@alexandernst
Copy link

@jquense Are you're still willing to merge this? If there is something missing, I can fork this branch and add it.

@alexandernst
Copy link

@jquense ping

2 similar comments
@alexandernst
Copy link

@jquense ping

@alexandernst
Copy link

@jquense ping

@jquense
Copy link
Owner

jquense commented Oct 16, 2020

can you resolve the conflict @abnersajr ?

@abnersajr
Copy link
Contributor Author

@jquense sure. I'm gonna do this.

@abnersajr
Copy link
Contributor Author

@jquense merge is done. Solved the conflicts adding to the sync and async version.

@jquense jquense merged commit 6096064 into jquense:master Oct 16, 2020
@jquense
Copy link
Owner

jquense commented Oct 16, 2020

thanks!

@alexandernst
Copy link

\o/

can we get a new minor release with this, please? 😄

@alexandernst
Copy link

@jquense Actually, I think this might require to bump the version up to 0.30.0 instead of 0.29.4.

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.

3 participants