Inconsistent runtime type checks in *.is_valid functions

[anderseknert]

IIRC, @philipaconrad mentioned this when working on the GraphQL built-in functions — creating an issue for completness sake.

package play

jv := json.is_valid(input.x)
yv := yaml.is_valid(input.x)
sv := semver.is_valid(input.x)
rv := regex.is_valid(input.x)
bv := base64.is_valid(input.x)
gv := graphql.is_valid(input.x, input.x)

With input as {"x": 5} (which is an invalid type for all of the functions) this package evaluates to:

{
    "rv": false,
    "sv": false
}

The documentation reports all of the above to return a boolean result, and while it also declares the required type of the args top be of type string, it's not documented what to expect back if that isn't the case.

I think we should either:

1. Be consistent and "return" undefined on other argument types in all of the above built-in functions
2. Be consistent and return false on other argument types in all of the above built-in functions
3. Document behavior for all of the above built-in functions when other argument type is provided

I personally like option 2, as I'd expect a function named "is_valid" to return false on any invalid input, but I'd be happy with any of the above.

[philipaconrad]

Thanks for filing this issue, @anderseknert!

My vote is for for a combination of 2 and 3: we ensure consistent behavior everywhere, and then document that behavior.

[anderseknert]

Changing this would be a "breaking" change to some extent, but I definitely doubt there's people relying on the difference. I agree with you though @philipaconrad — I do think that the docs are fine as they are if we go with this, as they're all documented as returning boolean?

[philipaconrad]

So, if I understand this correctly, the only change that needs to occur is that someone will need to sweep through the builtins and ensure that 2 (returning false consistently) is implemented across all the *.is_valid builtins? That seems quite doable. 😄

[anderseknert]

Sounds about right, @philipaconrad 👍

[philipaconrad]

The full set of *.is_valid builtins appears to be (as of v0.42.0):

Encoding:

• base64.is_valid
• json.is_valid
• yaml.is_valid

GraphQL:

• graphql.is_valid

Regex:

• regex.is_valid

Semver:

• semver.is_valid

Questionable additions to the list:

• io.jwt.verifyX functions?

[philipaconrad]

@anderseknert This actually brings up an interesting issue: Internally, the wrappers we use around the encoding-related builtins are deprecated, and different than the newer builtins. I think there's an opportunity for a refactoring here! 😃