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 attempt function #708

Merged
merged 1 commit into from
Aug 19, 2015
Merged

add attempt function #708

merged 1 commit into from
Aug 19, 2015

Conversation

danielb2
Copy link
Contributor

  • When Joi.assert doesn't throw, return the valid value.

@Marsup
Copy link
Collaborator

Marsup commented Aug 17, 2015

This is not the usual semantic of assert, not much of a fan.

@danielb2
Copy link
Contributor Author

This pattern is very common:

var result = Joi.validate(obj, schema);
Hoek.assert(!result.error, result.error && result.error.annotate());

Then result.valid goes on to be used elsewhere.

Do you have a better suggestion?

@danielb2
Copy link
Contributor Author

Also consider that this does not break the usual and expected semantic. It's just an improvement on how we can use it.

@Marsup
Copy link
Collaborator

Marsup commented Aug 17, 2015

Where is it common ?

@danielb2
Copy link
Contributor Author

Multiple different projects at Walmart, at other contracting gigs I've done (not even code I've written).
One variant is used in hapi itself.

https://github.com/hapijs/hapi/blob/master/lib/schema.js#L12

@hueniverse
Copy link
Contributor

I prefer the explicit version over this proposed shortcut. Assertion should never matter and removing them should not change anything other than less safety. The more verbose code above is actually easier to follow as it breaks what is happening into two clear steps.

@danielb2
Copy link
Contributor Author

Joi.assert() already does the two steps. It just doesn't do the assign.

As far as assertion never mattering; removing it in the code below would render the validate statement useless.

var result = Joi.validate(obj, schema);
Hoek.assert(!result.error, result.error && result.error.annotate());

If we said validation doesn't matter for anything but safety that would be one thing, but the proposed change is in the validation library itself, and in particular the assertion function. They are already coupled.

Also, one could still choose the more explicit form even after this change, if that's the preference.

@danielb2
Copy link
Contributor Author

var result = Joi.validate(obj, schema);
Joi.assert(obj, schema);

This is currently the laziest way of doing it, and the downside is obvious.

@danielb2 danielb2 changed the title make assert more valuable add attempt function Aug 18, 2015
@danielb2
Copy link
Contributor Author

After some discussion with @Marsup he suggested the word attempt to do what I'm looking for which I like. Assert remains the way it was, and the new function attempt is added here.

[attempt] Validates a value against a schema, returns valid object, and throws if validation fails

@kanongil
Copy link
Contributor

Awesome. I was about to suggest the same. I will update my code to use this once released.

@Marsup Marsup added the feature New functionality or improvement label Aug 19, 2015
@Marsup Marsup self-assigned this Aug 19, 2015
@Marsup
Copy link
Collaborator

Marsup commented Aug 19, 2015

Good to go ?

@danielb2
Copy link
Contributor Author

👍

@DavidTPate
Copy link
Contributor

Really glad this is being added, it will definitely lead to some cleaned up code for me. 👍

@Marsup Marsup added this to the 6.7.0 milestone Aug 19, 2015
Marsup added a commit that referenced this pull request Aug 19, 2015
@Marsup Marsup merged commit 4d8cc53 into hapijs:master Aug 19, 2015
@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
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants