Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make JSON::Validator::Util::is_bool return true when passed perl v5.36+ builtin booleans #275

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akarelas
Copy link
Contributor

@akarelas akarelas commented Aug 8, 2024

Summary

This PR allows newer Perls' (v5.36+) builtin core booleans to be validated as booleans. It's quite a necessary change to make, as when you include the line use v5.40 in your code file, you may not replace the meaning of true and false from perl's core booleans just by writing use Mojo::JSON 'true', 'false';. Therefore, it seems, core booleans need to be considered valid by all CPAN modules.

I did it by modifying slightly the JSON::Validator::Util::is_bool function, inspired by how JSON::PP recently achieved the same effect.

Motivation

My motivation was to resolve this bug of Mojolicious::Plugin::OpenAPI: jhthorsen/mojolicious-plugin-openapi#252

Tried my solution

I tested my modifications on a test Mojolicious website that uses Mojolicious::Plugin::OpenAPI, using both perl v5.34 (that doesn't have the builtin booleans) and perl v5.40 (that does), and the site works in both these cases.

I forgot to run prove -vl on it, or write tests with core booleans that will run only on perl v5.40. I will do so later today or tomorrow.

References

@jhthorsen
Copy link
Owner

I can't take the change, since too many tests fail.

@akarelas
Copy link
Contributor Author

We need to find out why those tests fail. Would they fail on any PR (even adding a blank line in a file)? If so, it's really a problem not to be attributed to me. If not, then I need to look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants