refactor: toBoolean does more; narrower predicates #125
Conversation
kalisjoshua
requested review from
brandonjpierce,
ArrayKnight and
belsrc
as code owners
kalisjoshua
force-pushed
the
refactor/122-bool-coverter-predicates
branch
from
46a0902 to
a2e1870
Compare
the toBoolean function centralizes the logic for coercing a value and allows the predicate functions to be more specific in what they validate rather than them simply being alais names to broad validation. closes #122
kalisjoshua
force-pushed
the
refactor/122-bool-coverter-predicates
branch
from
a2e1870 to
7b8e49a
Compare
|
I don't see them in here, were the benchmarks updated for these changes as well? |
|
I was waiting to update the bench tests till after we agree on a general direction; no need to benchmark something that isn't going to move forward. |
| return !( | ||
| !val || | ||
| `${val}`.trim().toLowerCase() === 'false' || | ||
| Number.parseFloat(val as string) === 0 |
There was a problem hiding this comment.
Why not wrap it in a string template literal? Since unknown is definitely not string
There was a problem hiding this comment.
Are you suggesting \${val}`instead of!val`? @ArrayKnight
There was a problem hiding this comment.
I think he meant Number.parseFloat(val as string) -> Number.parseFloat(`{val}`)
There was a problem hiding this comment.
That is why I asked. :)
That makes more sense.
There was a problem hiding this comment.
I can, and have changed it, but I remember my original reasoning being that as string is only for typescript so it will be removed in production while wrapping in a string template would carry into production.
There was a problem hiding this comment.
I would agree with that. Number.parseFloat({}) === NaN which doesn't === 0 so the logic checks out. Which I guess a case could be made, are we considering NaN as false?
There was a problem hiding this comment.
It is a good question. I asked myself that as well. What I fell back on is that Javascript considers is to be false so I felt that staying consistent with the language is best.
| */ | ||
| export const isOff = isFalse; | ||
| export const isOff = (val: unknown) => !val || test(['off'], val); |
There was a problem hiding this comment.
There's a little bit of a discrepancy between isOff and isOn, where isOff will accept any falsy value || off variants, instead of just false like how isOn only accepts true || on variants
I would expect them to be the same level of rigid/flexible. And I'd expect them both to accept things like 0 or 1 since that's relatively common term for on/off
|
Feels like maybe we also want something that is |
The converter I don't think it is a good idea to include all of the specific cases - yes + true + no or no + off + false - into a single call. My thought is that in cases where the comparison is for "on" or "off" it should not also match "yes" or "no". I am only able to draw from experience and conjecture without knowing some of the specific scenarios we run into. |
|
I decided to sever the relationship between First thought, for Second, for the predicates, |
| */ | ||
| export const isFalse = (val: unknown) => test(falseValues, val); | ||
| export const isFalse = (val: unknown) => test(listFalse, val); |
There was a problem hiding this comment.
I do like the separation of the three checks, although I think in our current use cases of this predicate we want to check against all of these. Would it perhaps make sense to do a isFalsey() function that checks isFalse() || isOff() || isNo() so we can do just a single check in userland (for general use cases)
There was a problem hiding this comment.
Is that a use case? Is a value likely to be "Off" or "No"?
In my experience I would expect a value to be either: "On" or "Off" or true or false but nothing else and another value might be of a different variant.
| */ | ||
| export function toBoolean(val: unknown) { | ||
| return isTrue(val); | ||
| return !( |
There was a problem hiding this comment.
Small nit for cleanliness (and any other long term checks we might do):
function notCheck(val) {
return !val;
}
function stringCheck(val) {
return string.trim().toLowerCase() === 'false';
}
function numberCheck(val) {
return Number.parseFloat(`${val}`) === 0;
}
export function toBoolean(val) {
return !(notCheck(val) || stringCheck(val) || numberCheck(val))
}
There was a problem hiding this comment.
Seems verbose for not much value and additional overhead - cognitive and execution - in my opinion. I would more often lean on comments for further documentation of why each exists than additional code to execute.
the toBoolean function centralizes the logic for coercing a value and allows the predicate functions to be more specific in what they validate rather than them simply being alais names to broad validation.
Closes #122
✅ Pull Request Checklist:
📝 Test Instructions:
❓ Does this PR introduce a breaking change?
The function
toBooleanno longer will consider specific values - 'no', 'off', 'n' to be "false" or 'on', 'yes', 'y' to be "true". If that is being relied on that assumption will break; no tests show this to be extant.💬 Other information