refactor: toBoolean does more; narrower predicates #125

kalisjoshua · 2024-12-04T16:36:07Z

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:

Included link to corresponding GitHub Issue.
The commit message follows conventional commit extended guidelines.
Added/updated unit tests and storybook for this change (for bug fixes / features).
Added/updated documentation (for bug fixes / features)
Filled out test instructions.

📝 Test Instructions:

❓ Does this PR introduce a breaking change?

Yes
No

The function toBoolean no 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

belsrc · 2024-12-04T20:45:48Z

I don't see them in here, were the benchmarks updated for these changes as well?

kalisjoshua · 2024-12-04T21:22:57Z

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.

packages/converters/src/to-boolean/index.ts

packages/predicates/src/is-noyes/index.ts

ArrayKnight · 2024-12-06T23:47:25Z

Feels like maybe we also want something that is isYes + isTrue + isOn & isNo + isFalse + isOff, like isTruthy & isFalsy?

kalisjoshua · 2024-12-09T18:26:59Z

Feels like maybe we also want something that is isYes + isTrue + isOn & isNo + isFalse + isOff, like isTruthy & isFalsy?

The converter toBoolean is the most liberal and could be combined with the more specific calls to gain those benefits I think.

if (toBoolean(val) || isYes(val)) {}

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.

kalisjoshua · 2024-12-09T18:46:49Z

I decided to sever the relationship between converters/toBoolean and predicates/is-noyes because I think they are not as compatible as might appear.

First thought, for toBoolean there should only two possible return values: true or false; it should never throw an error or return null or undefined. Therefor it needs to be liberal in it's conversion and that leads to the ethos of, "anything, that is something not explicitly false, is true." The other thought I had along these lines is, "if there is a list for true and list for false and we check against those two lists, then anything not in those lists would be an alternate return value." All of this leads me to believe that there is a small set of "false" values and anything outside of that set is not false, or true; at least for the purposes and domain of this type of function.

Second, for the predicates, toBoolean becomes problematic for truthy values because it's liberal criteria conflicts with the more restrictive ethos of is* functions. For instance because toBoolean('no') will evaluate to true then isYes('no') would evaluate to true without a more complicated check than ['y', 'yes', 1, 'true].includes(val). Also, I felt that it would be better to be consistent within is-noyes function implementations as well as limit interdependencies where not prohibitive.

brandonjpierce · 2024-12-11T20:22:49Z

packages/predicates/src/is-noyes/index.ts

 */
-export const isFalse = (val: unknown) => test(falseValues, val);
+export const isFalse = (val: unknown) => test(listFalse, val);


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)

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.

It is a use case for us on a project now, yeah, as we have seen both variations due to bad data and would like a single function to accommodate all

I've added the two additional functions - isAnyFalsy and isAnyTruthy - named that way to "distance" them from isFalse and isTrue. I don't have a strong feeling about this naming so if people hate it I can change it.

i think I prefer isFalsy/isTruthy, but I understand the motivation behind the current naming and am not opposed to leaving it

packages/converters/src/to-boolean/index.ts

brandonjpierce · 2024-12-17T01:56:11Z

@kalisjoshua will need a changeset, otherwise LGTM

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 · 2024-12-17T16:19:23Z

Changeset added.

orteth01 · 2024-12-17T22:07:57Z

packages/predicates/src/is-noyes/index.ts

 */
-export const isFalse = (val: unknown) => test(falseValues, val);
+export const isFalse = (val: unknown) => test(listFalse, val);


i think I prefer isFalsy/isTruthy, but I understand the motivation behind the current naming and am not opposed to leaving it

orteth01 · 2024-12-17T22:14:36Z

packages/predicates/src/is-noyes/index.test.ts

+    ${isOff}       | ${[...falsy, ' off', 'OFF ']}         | ${[...truthy, 'on', 'string with off', 'of']}
+    ${isOn}        | ${[...truthy, ' on', 'ON ']}          | ${[...falsy, 'of', 'string with on', 'o']}
+    ${isTrue}      | ${[...truthy, ' true']}               | ${[...falsy, 'any string', {}, [], /abc/]}
+    ${isYes}       | ${[...truthy, ' yes', 'YeS ', 'y']}   | ${[...falsy, 'no', 'string with yes', 2]}


just curious: does 'YeS ' reflect a value we actually see in the real world?

YeS, and also yES, and also YES 😆

kalisjoshua requested review from brandonjpierce, ArrayKnight and belsrc as code owners December 4, 2024 16:36

kalisjoshua linked an issue Dec 4, 2024 that may be closed by this pull request

Evolve toBoolean and isTrue (et.al) implementations #122

Closed

kalisjoshua force-pushed the refactor/122-bool-coverter-predicates branch 2 times, most recently from a2e1870 to 7b8e49a Compare December 4, 2024 18:37

ArrayKnight reviewed Dec 6, 2024

View reviewed changes

packages/converters/src/to-boolean/index.ts Outdated Show resolved Hide resolved

ArrayKnight reviewed Dec 6, 2024

View reviewed changes

packages/predicates/src/is-noyes/index.ts Outdated Show resolved Hide resolved

kalisjoshua closed this Dec 9, 2024

kalisjoshua reopened this Dec 9, 2024

brandonjpierce reviewed Dec 11, 2024

View reviewed changes

packages/converters/src/to-boolean/index.ts Show resolved Hide resolved

kalisjoshua force-pushed the refactor/122-bool-coverter-predicates branch from 55ed77a to 662b58c Compare December 17, 2024 16:08

refactor: toBoolean does more; narrower predicates

114c924

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 662b58c to 114c924 Compare December 17, 2024 16:17

orteth01 reviewed Dec 17, 2024

View reviewed changes

brandonjpierce approved these changes Dec 18, 2024

View reviewed changes

brandonjpierce merged commit 13f0d6c into main Dec 18, 2024
3 checks passed

brandonjpierce deleted the refactor/122-bool-coverter-predicates branch December 18, 2024 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: toBoolean does more; narrower predicates #125

refactor: toBoolean does more; narrower predicates #125

kalisjoshua commented Dec 4, 2024

belsrc commented Dec 4, 2024

kalisjoshua commented Dec 4, 2024

ArrayKnight commented Dec 6, 2024

kalisjoshua commented Dec 9, 2024 •

edited

Loading

kalisjoshua commented Dec 9, 2024

brandonjpierce Dec 11, 2024

kalisjoshua Dec 11, 2024

brandonjpierce Dec 17, 2024

kalisjoshua Dec 17, 2024

orteth01 Dec 17, 2024

brandonjpierce commented Dec 17, 2024 •

edited

Loading

kalisjoshua commented Dec 17, 2024

orteth01 Dec 17, 2024

orteth01 Dec 17, 2024 •

edited

Loading

brandonjpierce Dec 18, 2024

refactor: toBoolean does more; narrower predicates #125

refactor: toBoolean does more; narrower predicates #125

Conversation

kalisjoshua commented Dec 4, 2024

✅ Pull Request Checklist:

📝 Test Instructions:

❓ Does this PR introduce a breaking change?

💬 Other information

belsrc commented Dec 4, 2024

kalisjoshua commented Dec 4, 2024

ArrayKnight commented Dec 6, 2024

kalisjoshua commented Dec 9, 2024 • edited Loading

kalisjoshua commented Dec 9, 2024

brandonjpierce Dec 11, 2024

Choose a reason for hiding this comment

kalisjoshua Dec 11, 2024

Choose a reason for hiding this comment

brandonjpierce Dec 17, 2024

Choose a reason for hiding this comment

kalisjoshua Dec 17, 2024

Choose a reason for hiding this comment

orteth01 Dec 17, 2024

Choose a reason for hiding this comment

brandonjpierce commented Dec 17, 2024 • edited Loading

kalisjoshua commented Dec 17, 2024

orteth01 Dec 17, 2024

Choose a reason for hiding this comment

orteth01 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

brandonjpierce Dec 18, 2024

Choose a reason for hiding this comment

kalisjoshua commented Dec 9, 2024 •

edited

Loading

brandonjpierce commented Dec 17, 2024 •

edited

Loading

orteth01 Dec 17, 2024 •

edited

Loading