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

allow param defaults to be set by the param validations #1103

Merged
merged 2 commits into from
Feb 9, 2017

Conversation

kturney
Copy link
Contributor

@kturney kturney commented Feb 9, 2017

I found it surprising while making an extension that the defaults I set in the params validation weren't being applied.

Copy link
Collaborator

@Marsup Marsup left a comment

Choose a reason for hiding this comment

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

Good catch.

lib/index.js Outdated
@@ -298,7 +298,11 @@ internals.root = function () {
}

if (validateArgs) {
joi.assert(arg, validateArgs);
const res = joi.validate(arg, validateArgs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you replace with arg = joi.attempt(arg, validateArgs) ?

@kturney
Copy link
Contributor Author

kturney commented Feb 9, 2017

Maybe you'd rather I use joi.attempt rather than joi.validate? Either way, it seems the "throws error when validations fails" branch wasn't being hit.

@Marsup Marsup self-assigned this Feb 9, 2017
@Marsup Marsup added the bug Bug or defect label Feb 9, 2017
@Marsup
Copy link
Collaborator

Marsup commented Feb 9, 2017

What do you mean not being hit ? The coverage is still 100%.

@kturney
Copy link
Contributor Author

kturney commented Feb 9, 2017

I needed to add a few expects to cause the if (res.error) { throw res.error } branch to be hit, which is implicit with joi.assert and joi.attempt (unless I'm missing something).

@Marsup
Copy link
Collaborator

Marsup commented Feb 9, 2017

Ah ok, then you reached a better coverage, that's great.

@Marsup Marsup merged commit f1db15f into hapijs:master Feb 9, 2017
@Marsup Marsup added this to the 10.2.2 milestone Feb 9, 2017
@kturney
Copy link
Contributor Author

kturney commented Feb 9, 2017

Thanks for the quick turn around and all of your hard work! :-)

@Marsup
Copy link
Collaborator

Marsup commented Feb 9, 2017

It's published.

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants