-
Notifications
You must be signed in to change notification settings - Fork 19
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
Handle not nodes when resolving conditions #226
base: main
Are you sure you want to change the base?
Conversation
0c46cc8
to
f350ba7
Compare
src/typeEvaluator/typeEvaluate.ts
Outdated
return value.value | ||
} | ||
|
||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this makes sense for AccessAttribute
. E.g. if evaluating AccessAttribute
returns unknown
then we want to return undefined
.
Also: Can't resolveCondition
just be like this? 🤔
const value = walk({node: expr, scope})
if (value.type === 'boolean') return value.value
if (value.type === 'null') return false
return undefined
And then we depend on the regular walk
function to handle the different operators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on returning undefined on unknown!
We would need to refactor it a bit as handleOpCall depends on resolveCondition 🤔 - I can have a look outside this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. This doesn't seem to have been addressed in the latest re-review? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it to default to undefined
but also return false on array, object, null
ed9b958
to
184eb8a
Compare
184eb8a
to
aee1e8d
Compare
No description provided.