-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adds NOT and XOR triggers for conditions #2816
Conversation
Adds unit tests Fixes linting issues
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.
Pulled this down locally and I've got nothing to add - good to go.
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.
@Shefali looks great, two tiny changes that I will file followups for.
let trueCount = 0; | ||
for (let key in resultMap) { | ||
if (resultMap.hasOwnProperty(key)) { | ||
if (resultMap[key]) { |
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 think that generally speaking just checking for "truthiness" is a bit vague and can lead to subtle bugs. I'd prefer to be explicit here and check if (resultMap[key]) === true
or if (resultMap[key]) !== undefined
- whatever the intended behavior is here.
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.
Done.
'any': 'when any criteria are met', | ||
'all': 'when all criteria are met', | ||
'not': 'when no criteria are met', | ||
'xor': 'when only one criteria is met' |
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.
Being pedantic, but it should be criterion
singular (as we correctly have it elsewhere in our code).
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.
Done.
Reviewer Checklist
|
Resolves #2811
Author Checklist