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.

Sign up for GitHub

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

Jump to bottom

Add method to return the error without throwing #176

Closed
wants to merge 1 commit into from
Closed

Add method to return the error without throwing #176

wants to merge 1 commit into from

Conversation

aqzhyi
Copy link

@aqzhyi aqzhyi commented Mar 7, 2020 •
edited by sindresorhus
Loading

Hi, the project joi has API validate it returns {error, value} such as:

let schema = Joi.string().empty('');
schema.validate(''); // returns { error: null, value: undefined }

schema = schema.empty();
schema.validate(''); // returns { error: "value" is not allowed to be empty, value: '' }

Or .error(new Error(...))

const schema = Joi.string()
  .error(new Error('Was REALLY expecting a string'));

schema.validate(3);
// returns Error('Was REALLY expecting a string')

So I make API validate and createValidate it also returns {error, value}.

What do you think?

Fixes #169

@aqzhyi aqzhyi mentioned this pull request Apr 30, 2020
Open
GentileFulvio
GentileFulvio suggested changes May 1, 2020
View reviewed changes
Copy link
Contributor

@GentileFulvio GentileFulvio left a comment

Choose a reason for hiding this comment

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

I have one suggestion. Not sure how @sindresorhus sees this. About the code coverage it seems like you haven't covered all cases on this line.

image.

I think it should be enough to test the createValidate without a label as well. Something like

const checkNickname = ow.createValidate(ow.string.minLength(3));

test/test.ts

t.notThrows(() => {
const {error, value} = ow.validate(valueCauseError, ow.string);
t.true(error instanceof Error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure but wouldn't it make more sense for error to be of type string and just contain the message of the error ?

@sindresorhus sindresorhus changed the title Add method to return the error without throwing #169 Add method to return the error without throwing May 6, 2020
sindresorhus
sindresorhus requested changes May 6, 2020
View reviewed changes
readme.md
@@ -107,6 +107,30 @@ checkPassword('foo');
//=> ArgumentError: Expected string `password` to have a minimum length of `6`, got `foo`
```

### ow.validate(predicate)
Copy link
Owner

Choose a reason for hiding this comment

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

This needs a better name. Creating a "validator" is what ow.create does. So this creates confusion.

readme.md

### ow.createValidate(label, predicate)

Create a reusable validator with a specific `label`. It returns error and value.
Copy link
Owner

Choose a reason for hiding this comment

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

This description is the same as ow.validate. I think you need to make all of this much clearer.

source/index.ts
@param value - Value to test.
@param predicate - Predicate to test against.
*/
validate<T>(value: T, predicate: BasePredicate<T>): {error: Error | null; value: T};
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
validate<T>(value: T, predicate: BasePredicate<T>): {error: Error | null; value: T};
validate<T>(value: T, predicate: BasePredicate<T>): {error: Error?; value: T};

test/test.ts
test('reusable validator', t => {
const checkUsername = ow.create(ow.string.minLength(3));
const checkNickname = ow.createValidate('nickname', ow.string.minLength(3));
Copy link
Owner

Choose a reason for hiding this comment

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

This should be in a separate test.

test/test.ts

t.notThrows(() => {
const {error, value} = checkNickname('foo');
t.true(error === null);
Copy link
Owner

Choose a reason for hiding this comment

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

Use t.is

source/index.ts
@@ -108,6 +140,40 @@ Object.defineProperties(ow, {

test(value, label ?? (labelOrPredicate as string), predicate as BasePredicate<T>);
}
},
validate: {
value: <T>(value: T, labelOrPredicate: unknown, predicate?: BasePredicate<T>) => {
Copy link
Owner

Choose a reason for hiding this comment

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

labelOrPredicate should not be unknown.

@sindresorhus
Copy link
Owner

@hilezir Bump

@sindresorhus
Copy link
Owner

Closing for lack of response.

#169 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GentileFulvio GentileFulvio GentileFulvio requested changes

@sindresorhus sindresorhus sindresorhus requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add method to return the error without throwing
3 participants
@aqzhyi @sindresorhus @GentileFulvio