Skip to content

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

Merged
bcoe merged 3 commits intoyargs:masterfrom
mathieubergeron:fix-implies-key-exists-check
Jul 28, 2021
Merged

fix: implies should not fail when implied key's value is 0, false or empty string#1985
bcoe merged 3 commits intoyargs:masterfrom
mathieubergeron:fix-implies-key-exists-check

Conversation

@mathieubergeron
Copy link
Copy Markdown
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
Copy Markdown
Member

bcoe commented Jul 15, 2021

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

@bcoe
Copy link
Copy Markdown
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 😄

Comment thread test/validation.cjs Outdated
@jly36963
Copy link
Copy Markdown
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.

3 participants