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 multilple valid types #7207

Merged
merged 2 commits into from
Oct 18, 2018

Conversation

jamietre
Copy link
Contributor

@jamietre jamietre commented Oct 18, 2018

Summary

Split from #7194
Prerequisite to resolving #7180

This PR provides a syntax for jest-validate to accept more than one example for a single config value, which can be of different types. This allows overloading config options to accept multiple types.

Test plan

  • existing unit tests pass (except as expected where internal config changed types)
  • added unit tests to cover new valid and error conditions

@jamietre jamietre force-pushed the jamiet/jest-validate-multiple branch 3 times, most recently from e46bf2d to 5c25668 Compare October 18, 2018 13:50
@jamietre jamietre changed the title Allow mutilple valid types Allow multilple valid types Oct 18, 2018
CHANGELOG.md Outdated
@@ -15,6 +15,7 @@
- `[jest-runtime]` If `require` fails without a file extension, print all files that match with one ([#7160](https://github.com/facebook/jest/pull/7160))
- `[jest-haste-map]` Make `ignorePattern` optional ([#7166](https://github.com/facebook/jest/pull/7166))
- `[jest-runtime]` Remove `cacheDirectory` from `ignorePattern` for `HasteMap` if not necessary ([#7166](https://github.com/facebook/jest/pull/7166))
- `[jest-validate]` Add syntax to validate multiple permitted types
Copy link
Member

Choose a reason for hiding this comment

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

could you include a link to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@SimenB SimenB requested a review from thymikee October 18, 2018 14:00
@jamietre jamietre force-pushed the jamiet/jest-validate-multiple branch from 5c25668 to 4c84514 Compare October 18, 2018 14:06
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

That's cool! I left some nits to fix.
I'm still thinking about syntax. What do you think is better:

key: [MultipleValidOptions, a, b]

or

key: MultipleValidOptions(a, b)

I'd prefer the second one, but no hard opinions on that.

packages/jest-validate/src/condition.js Outdated Show resolved Hide resolved
packages/jest-validate/src/errors.js Outdated Show resolved Hide resolved
const validTypes = conditions
.map(getType)
.filter(uniqueFilter())
.join(' or ');
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about

const validTypes = Array.from(new Set(conditions.map(getType))).join(' or ');

@jamietre
Copy link
Contributor Author

jamietre commented Oct 18, 2018

On syntax, I think I was channeling a JSON-compatible config, but I guess that doesn't really matter if we need to use a Symbol to make it deterministic.

If we use a function, it's still going to have to return something that can be identified by the parser, so we'd still probably do something like [_MultipleValidOptions, ...] anyway internally. That seems okay, though seems to add a small amount of complexity to improve the usage a little bit.

Actually, I agree this is better, just because it eliminates ambiguity at the use site between this and a real array.

@thymikee
Copy link
Collaborator

I think it should be as simple as:

function MultipleValidOptions(...args) {
  return args
}

MultipleValidOptions.$$type = Symbol('jest-validate-multiple-valid-options');

You get an array of values just like in current implementation

@jamietre
Copy link
Contributor Author

jamietre commented Oct 18, 2018

I think it should be as simple as:

function MultipleValidOptions(...args) {
  return args
}

MultipleValidOptions.$$type = Symbol('jest-validate-multiple-valid-options');

You get an array of values just like in current implementation

Not sure I understand... if someone codes

const validOptions = {
   key: MultipleValidOptions(1, '2')
};

using this definition, then validOptions.key is [1, '2'] which is simply an array, unless there's some magic meaning to $$type I don't know about?

@jamietre
Copy link
Contributor Author

Changed per the API you proposed; if there's a more trivial implementation I'm missing here then happy to revise.

@jamietre jamietre force-pushed the jamiet/jest-validate-multiple branch from d36ab38 to 29e4073 Compare October 18, 2018 15:53
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants