Add support for truthy/falsy boolean values #998

WesTyler · 2016-10-05T14:15:25Z

This commit resolves #991 by adding the following support:

Implements Joi.boolean().truthy() and Joi.boolean().falsy()
- Both methods accept either a string, or an array of strings.
- Both methods utilize Hoek.assert to ensure that the values passed are strings.
- Both methods have 100% test coverage.
- Both methods are documented in API.md.

DavidTPate · 2016-10-05T14:26:24Z

API.md

@@ -808,6 +810,24 @@ const boolean = Joi.boolean();
 boolean.validate(true, (err, value) => { });
 ```

+#### `boolean.truthy(value)`
+
+Allows for additional strings to be considered valid boolean values in addition to default 'truthy' strings: ['true', 'yes', '1', 'on']. Accepts a string or an array of strings.


Personally, I don't think this should be additional as I want to be able to be explicit about it. Instead I think this should override the valid truthy values instead of just add new ones.

The same goes for the falsey one below.

Yeah I get that. It is currently (in this PR) implemented as additive, so that I didn't nuke the strings that are already type-coerced by the "vanilla" boolean schema type. Here's where the "additive" piece is in the code: https://github.com/hapijs/joi/pull/998/files#diff-3b86748616af947ebd7969eedec0062aR76

I'm totally down for overriding the default type-coerced strings. I was just trying to play nice with what was already in place. Just let me know what would be preferred and I will update accordingly.

Yeah, I figured that was the case @WesTyler. It's definitely just my personal preference here not sure that one method is more correct than the other.

Not sure of the opinion that @Marsup has on this, but I definitely defer to him. Just wanted to state my preference.

Like I said in the issue, I don't mind it being a breaking change, so default true/false and additive calls to stay consistent with valid/invalid makes the most sense to me.

DavidTPate · 2016-10-05T14:27:03Z

API.md

+
+```js
+const boolean = Joi.boolean().truthy('Y');
+boolean.validate('Y', (err, value) => { });


Doing boolean.validate('Y', (err, value) => { }); // valid or something else might help to show that this works.

Sure thing. On it, thanks.

AdriVanHoudt · 2016-10-05T14:31:43Z

Why only allow string values?

DavidTPate · 2016-10-05T14:32:39Z

lib/boolean.js

+                return value.toLowerCase();
+            });
+        }
+        else {


Why not just cast the value to an array if it isn't one currently? That way you don't have the code repeated. So before line 62 something like: truthyValue = Array.isArray(truthyValue) ? truthyValue : [];

Good call. I'll do that along with the API.md update above. Thanks

This is actually inconsistent with how we do it everywhere else, see https://github.com/hapijs/joi/blob/master/lib/any.js#L235.

WesTyler · 2016-10-05T14:35:00Z

@AdriVanHoudt Simply because that was what was discussed in the feature request issue. And I thought a simple initial implementation would be best for a new feature PR. It can always be extended in the future if the need for non-string values arises.

AdriVanHoudt · 2016-10-05T14:58:10Z

@WesTyler I see your point, although feature wise it would seem weird to limit it (if you are not involved with the original issue)

Marsup · 2016-10-05T22:56:07Z

Agreed with @AdriVanHoudt here, I thought my pov was clear in the original issue, sorry for that.

WesTyler · 2016-10-06T13:26:58Z

@Marsup Ah, gotcha. I misunderstood what you meant in the issue about it being a breaking change. Updating right now per your 3 comments above. Thanks for the input!

WesTyler · 2016-10-06T13:55:23Z

Now that I just pushed that though, I'm wondering if the ._inner.truthySet (and falsy) should utilize the Set class defined in the Any internals object? That way it can take advantage of the getter/setter and has, rather than relying on push and indexOf.

Thoughts?

Marsup · 2016-10-12T19:16:12Z

@WesTyler yes, this might be a good idea. Careful with cloning though, you'll have to implement a slice equivalent (https://github.com/hapijs/joi/blob/master/lib/any.js#L110) because _inner properties are meant to be arrays.

Marsup · 2016-10-12T19:20:23Z

This might also be a challenge for concat() and describe() which are currently lacking in the PR. Joi is a complex beast :)

WesTyler · 2016-10-12T23:16:05Z

Alright, I extracted the Set class out of any.js into its own file, and implemented it for the boolean inners.

I also implemented Set.concat and Set.slice and added corresponding tests.

I added boolean-specific tests around schema.concat() and schema.describe() as well.

Marsup · 2016-10-13T09:54:43Z

lib/set.js

+
+const Ref = require('./ref');
+
+const Set = class {


Could you make this exports.Set ?

@WesTyler oh my, sorry, I was thinking module.exports = class, just bypassing the local constant.

Haha, my bad. Yeah I'll do it that way.

Marsup · 2016-10-13T09:55:09Z

lib/boolean.js

+        return result;
+    }
+
+    _truthy() {


Why did you split both truthy and falsy ?

Just following the pattern set with _allow and allow in Any. I can un-split them if you prefer though; going to leave it as-is unless you specifically want them combined @Marsup :)

Marsup · 2016-10-13T10:00:02Z

lib/boolean.js


-            result.value = (value === 1 ? true
-                    : (value === 0 ? false : value));
+        if (this._inner._truthySet.values().length) {


values() is already a copy, so I guess description.truthyValues = this._inner._truthySet.values() is enough.

Marsup · 2016-10-13T10:01:44Z

lib/set.js

+    slice() {
+
+        const newSet = new Set();
+        newSet.merge(this);


merge will be slower, a simple newSet._set = this._set.slice() would be better.

Alright cool. Wasn't sure if it was kosher to access the private _set from outside the class. Definitely agree with you though. I'm on it, along with other requested revisions.

Marsup · 2016-10-13T10:08:15Z

lib/set.js

+    concat(source) {
+
+        const newSet = new Set();
+        newSet.merge(this);


Same here, probably newSet._set = this._set.concat(source._set).

…fy boolean.describe

WesTyler · 2016-10-13T15:56:34Z

I thought I had already left this comment, but I don't see it now. So here goes again.

The Set class has been refactored per change requests above. I left boolean.truthy/falsy as-is for now. Not a big deal for me to combine _truthy/truthy and _falsy/falsy, I just was trying to mirror _allow/allow as closely as possible.

Marsup · 2016-10-13T15:59:40Z

Can't remember why _allow is this way but I see no reason to do that.

WesTyler · 2016-10-13T16:03:57Z

Alright, I'll update truthy/falsy then. Would you like me to go ahead and re-combine _allow/allow in Any while I'm at it?

Marsup · 2016-10-13T16:06:25Z

Nah let's keep this PR focused, I'll do it someday :)

WesTyler · 2016-10-13T16:08:37Z

Alright, pushed the removal of Boolean's _truthy/_falsy pattern.

WesTyler · 2016-10-18T14:04:45Z

@Marsup Is there anything else you're waiting on me to fix/update/refactor?

Not trying to rush you, just making sure you're not waiting on me :)

Marsup · 2016-10-22T11:00:05Z

Almost perfect, I'll deal with the bits I don't like myself. Thanks a lot !
Know that this won't be published right away though, I'd like to tackle the other pending breaking change on the same release.

Marsup · 2016-10-22T12:09:15Z

LMK if you see anything shocking to you in my cleanup.

WesTyler · 2016-10-22T18:45:35Z

Awesome! Thanks for the patience and guidance on this! :D
And yeah, I figured you would want to bundle several breaking changes in together before publishing. 👍

Cleanup makes sense to me.

lock · 2020-01-09T10:03:46Z

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

Add support for truthy/falsy boolean values

da5de23

WesTyler closed this Oct 5, 2016

WesTyler reopened this Oct 5, 2016

DavidTPate reviewed Oct 5, 2016

View reviewed changes

Simplify bool.truthy/falsy methods per feedback

e7ee3e5

Refactor boolean.truthy/falsy to be breaking changes

2eaeb4f

Fix typo in API.md

fc457b7

Marsup added feature New functionality or improvement breaking changes Change that can breaking existing code labels Oct 12, 2016

Marsup self-assigned this Oct 12, 2016

Refactor boolean.truthy/falsy to utilize Set class

566cdc1

Marsup requested changes Oct 13, 2016

View reviewed changes

WesTyler added 2 commits October 13, 2016 10:27

Refactor Set methods to utilize Array methods for performance; simpli…

4c81267

…fy boolean.describe

Update Set class export style from exports.Set to module.exports

5649c87

Remove boolean _truthy/_falsy pattern

752cf85

Marsup approved these changes Oct 22, 2016

View reviewed changes

Marsup merged commit 2eed6a8 into hapijs:master Oct 22, 2016

Marsup added a commit that referenced this pull request Oct 22, 2016

Cleanup for #998.

3d31dc4

Marsup added this to the 10.0.0 milestone Nov 6, 2016

WesTyler mentioned this pull request Nov 11, 2016

Clean up any.valid code #1032

Merged

galenandrew mentioned this pull request Dec 13, 2016

10.0.0 Release Notes #1037

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

Add support for truthy/falsy boolean values #998

Add support for truthy/falsy boolean values #998

Conversation

WesTyler commented Oct 5, 2016

DavidTPate Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdriVanHoudt commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler commented Oct 5, 2016

AdriVanHoudt commented Oct 5, 2016

Marsup commented Oct 5, 2016

WesTyler commented Oct 6, 2016

WesTyler commented Oct 6, 2016

Marsup commented Oct 12, 2016

Marsup commented Oct 12, 2016

WesTyler commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler commented Oct 13, 2016

Marsup commented Oct 13, 2016

WesTyler commented Oct 13, 2016

Marsup commented Oct 13, 2016

WesTyler commented Oct 13, 2016

WesTyler commented Oct 18, 2016

Marsup commented Oct 22, 2016

Marsup commented Oct 22, 2016

WesTyler commented Oct 22, 2016

lock bot commented Jan 9, 2020

DavidTPate Oct 5, 2016 •

edited

Loading

WesTyler Oct 13, 2016 •

edited

Loading