Skip to content

Commit

Permalink
Code review
Browse files Browse the repository at this point in the history
  • Loading branch information
Jamie Treworgy committed Oct 18, 2018
1 parent 4c84514 commit 29e4073
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 30 deletions.
4 changes: 2 additions & 2 deletions packages/jest-validate/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ You will find examples of `condition`, `deprecate`, `error`, `unknown`, and `dep

- it matches the JavaScript type of the example value, e.g. `string`, `number`, `array`, `boolean`, `function`, or `object`
- it is `null` or `undefined`
- the example value is an array where the first element is the special value `MultipleValidOptions`, and the value matches the type of _any_ of the other values in the array
- it matches the Javascript type of any of arguments passed to `MultipleValidOptions(...)`

The last condition is a special syntax that allows validating where more than one type is permissible; see example below. It's acceptable to have multiple values of the same type in the example, so you can also use this syntax to provide more than one example. When a validation failure occurs, the error message will show all other values in the array as examples.

Expand Down Expand Up @@ -145,7 +145,7 @@ import {MultipleValidOptions} from 'jest-validate';

validate(config, {
// `bar` will accept either a string or a number
bar: [MultipleValidOptions, 'string is ok', 2],
bar: MultipleValidOptions('string is ok', 2),
});
```

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ exports[`Repeated types within multiple valid examples are coalesced in error re
"<red><bold><bold>●<bold> Validation Error</>:</>
<red></>
<red> Option <bold>\\"foo\\"</> must be of type:</>
<red> <bold><green>string or number<red></></>
<red> <bold><green>string<red></> or <bold><green>number<red></></>
<red> but instead received:</>
<red> <bold><red>boolean<red></></>
<red></>
Expand Down Expand Up @@ -138,7 +138,7 @@ exports[`reports errors nicely when failing with multiple valid options 1`] = `
"<red><bold><bold>●<bold> Validation Error</>:</>
<red></>
<red> Option <bold>\\"foo\\"</> must be of type:</>
<red> <bold><green>string or array<red></></>
<red> <bold><green>string<red></> or <bold><green>array<red></></>
<red> but instead received:</>
<red> <bold><red>number<red></></>
<red></>
Expand Down
6 changes: 3 additions & 3 deletions packages/jest-validate/src/__tests__/validate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ test('works with custom deprecations', () => {

test('works with multiple valid types', () => {
const exampleConfig = {
foo: [MultipleValidOptions, 'text', ['text']],
foo: MultipleValidOptions('text', ['text']),
};

expect(
Expand Down Expand Up @@ -239,7 +239,7 @@ test('works with multiple valid types', () => {

test('reports errors nicely when failing with multiple valid options', () => {
const exampleConfig = {
foo: [MultipleValidOptions, 'text', ['text']],
foo: MultipleValidOptions('text', ['text']),
};

expect(() =>
Expand All @@ -254,7 +254,7 @@ test('reports errors nicely when failing with multiple valid options', () => {

test('Repeated types within multiple valid examples are coalesced in error report', () => {
const exampleConfig = {
foo: [MultipleValidOptions, 'foo', 'bar', 2],
foo: MultipleValidOptions('foo', 'bar', 2),
};

expect(() =>
Expand Down
14 changes: 8 additions & 6 deletions packages/jest-validate/src/condition.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

const toString = Object.prototype.toString;

export const MultipleValidOptions = Symbol('JEST_MULTIPLE_VALID_OPTIONS');
const MultipleValidOptionsSymbol = Symbol('JEST_MULTIPLE_VALID_OPTIONS');

function validationConditionSingle(option: any, validOption: any): boolean {
return (
Expand All @@ -19,19 +19,21 @@ function validationConditionSingle(option: any, validOption: any): boolean {
);
}

export function getConditions(validOption: any) {
export function getValues(validOption: any) {
if (
Array.isArray(validOption) &&
validOption.length &&
validOption[0] === MultipleValidOptions
validOption[0] === MultipleValidOptionsSymbol
) {
return validOption.slice(1);
}
return [validOption];
}

export function validationCondition(option: any, validOption: any): boolean {
return getConditions(validOption).some(e =>
validationConditionSingle(option, e),
);
return getValues(validOption).some(e => validationConditionSingle(option, e));
}

export function MultipleValidOptions(...args: Array<any>) {
return [MultipleValidOptionsSymbol, ...args];

This comment has been minimized.

Copy link
@thymikee

thymikee Oct 18, 2018

Collaborator

I thought about adding a special $$type field to this fn so you can check if the value is typeof function and if $$type === symbol, instead of converting this to an array and asserting on its first value

This comment has been minimized.

Copy link
@jamietre

jamietre Oct 18, 2018

Contributor

Oh, I can't think how that would work; you'd have to assign the function itself, in which case you wouldn't be invoking it with your desired arguments.

This comment has been minimized.

Copy link
@thymikee

thymikee Oct 18, 2018

Collaborator

Nvm, this is the implementation detail, and we already have desired API I'm place

}
23 changes: 6 additions & 17 deletions packages/jest-validate/src/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type {ValidationOptions} from './types';
import chalk from 'chalk';
import getType from 'jest-get-type';
import {formatPrettyObject, ValidationError, ERROR} from './utils';
import {getConditions} from './condition';
import {getValues} from './condition';

export const errorMessage = (
option: string,
Expand All @@ -21,16 +21,15 @@ export const errorMessage = (
options: ValidationOptions,
path?: Array<string>,
): void => {
const conditions = getConditions(defaultValue);
const validTypes = conditions
.map(getType)
.filter(uniqueFilter())
.join(' or ');
const conditions = getValues(defaultValue);
const validTypes: Array<string> = Array.from(
new Set(conditions.map(getType)),
);

const message = ` Option ${chalk.bold(
`"${path && path.length > 0 ? path.join('.') + '.' : ''}${option}"`,
)} must be of type:
${chalk.bold.green(validTypes)}
${validTypes.map(e => chalk.bold.green(e)).join(' or ')}
but instead received:
${chalk.bold.red(getType(received))}
Expand All @@ -54,13 +53,3 @@ function formatExamples(option: string, examples: Array<any>) {
`);
}

function uniqueFilter() {
const seen: {[string]: any} = {};
return function(key) {
if (seen[key]) {
return false;
}
return (seen[key] = true);
};
}

0 comments on commit 29e4073

Please sign in to comment.