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

fix: implies should not fail when implied key's value is 0, false or empty string #1985

Merged
merged 3 commits into from Jul 28, 2021

Conversation

mathieubergeron
Copy link
Contributor

@mathieubergeron mathieubergeron commented Jul 14, 2021

This is an attemps at fixing #1091 and #1203.

The issue

The documentation for implies is:

Given the key x is set, it is required that the key y is set.

But this check fails when the value of the key y is 0, false, or empty string.

When y value is false, we might want implies to continue failing though. I'd like to have your input on that, and I will adjust that PR accordingly.

The issue came from keyExists which, instead of always returning a boolean, could returned the value of the key. That value is later on used like so:

if (key && !value) {
  implyFail.push(` ${origKey} -> ${origValue}`);
}

Thus, that if resolved to true when value was 0, false or "".

The fix

Having keyExists always returning a boolean seems more robust. I used Object.hasOwnProperty to check for presence of the key instead.

Let me know if you think that keyExists should still return false when the value of the key is false.

Thanks :)

@bcoe
Copy link
Member

bcoe commented Jul 15, 2021

@mathieubergeron thanks for the contribution 👍 will make an effort to give review soon.

@bcoe
Copy link
Member

bcoe commented Jul 15, 2021

@jly36963 if you happen to beat me to code review, at a glance this contribution looked solid to me 😄

test/validation.cjs Outdated Show resolved Hide resolved
@jly36963
Copy link
Contributor

@bcoe I tested it locally with and without his changes, his fix works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants