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

only emit equal when needed: addresses #1852 #1853

Closed
wants to merge 7 commits into from

Conversation

cscheid
Copy link

@cscheid cscheid commented Dec 16, 2021

What issue does this pull request resolve?
This addresses issue #1852 .

What changes did you make?
I added a check to the enum types before emitting the requirement for equal.

Is there anything that requires more attention while reviewing?
Not that I'm aware of.

@cscheid
Copy link
Author

cscheid commented Dec 16, 2021

I had to add a (schema as any[]) cast to schema.some() in order to pass eslint, but that seems safe since the code just above it already assumes an array with schema.length

@cscheid
Copy link
Author

cscheid commented Dec 16, 2021

(Once more with feeling) An explicit check for Array.isArray, and now npm run test passes locally for me.

@@ -20,7 +21,11 @@ const def: CodeKeywordDefinition = {
const {gen, data, $data, schema, schemaCode, it} = cxt
if (!$data && schema.length === 0) throw new Error("enum must have non-empty array")
const useLoop = schema.length >= it.opts.loopEnum
const eql = useFunc(gen, equal)
const needsEql =
Copy link
Member

Choose a reason for hiding this comment

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

I've made a small correction, but in general this is a brittle approach - if the condition of usage changes, these conditions would have to be updated too, and there may be edge cases that would not be caught by the tests.

A much better approach would be to simply call useFunc in the place where eql is actually used.

Copy link
Member

Choose a reason for hiding this comment

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

so probably just make a memoized getEql function and replace eql with getEql() in two places.

@epoberezkin
Copy link
Member

closing in favour of #1922

epoberezkin added a commit that referenced this pull request Mar 22, 2022
* fix: emit equal when needed - alternative to #1853

* fix: prettier errors

* fix: tslint errors

* Update lib/vocabularies/validation/enum.ts

* Update lib/vocabularies/validation/enum.ts

Co-authored-by: Evgeny Poberezkin <2769109+epoberezkin@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants