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

JSON::Validator 3 changes #45

Merged
merged 5 commits into from Jan 8, 2019

Conversation

Projects
None yet
3 participants
@mohawk2
Copy link
Contributor

mohawk2 commented Jan 7, 2019

JSON::Validator::OpenAPI is gone, use JSON::Validator::OpenAPI::Mojolicious

format functions must return undef when ok

Coerce to real JSON booleans, not 'true'/'false' as JV fixed a bug. Previously coercion did not coerce ie mutate the passed-in values, now it does and it doesn't recognise the strings as boolean values.

Also update the deps, and eliminate a surprising duplicate blog spec in t/helpers.t.

Fixes #44.

mohawk2 added some commits Jan 7, 2019

JSON::Validator 3 changes
JSON::Validator::OpenAPI is gone, use JSON::Validator::OpenAPI::Mojolicious

format functions must return undef when ok

Coerce to real JSON booleans, not 'true'/'false' as JV fixed a
bug. Previously coercion did not work right, now it does and it doesn't
recognise the strings as boolean values.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 7, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 8916f8b on mohawk2:fix-jv3 into 4cb8768 on preaction:master.

mohawk2 and others added some commits Jan 7, 2019

remove need for Cpanel::JSON::XS by fixing test
This is weird. I don't know why these particular values need to be
boolean objects instead of 0/1. I would have expected all of the values
to be boolean objects now that output is being coerced, but that's not
what is happening here...
@preaction

This comment has been minimized.

Copy link
Owner

preaction commented Jan 8, 2019

I added a commit that removes the need for Cpanel::JSON::XS by changing the test's data, but I have no clue why my commit works. I would've expected that if I were to have changed all the 0 and 1 being used to compare as booleans to the true and false provided by Mojo::JSON, that would've worked. But that didn't work. Instead, I had to change some specific values to true and false (and sometimes change them back. It seems like JSON::Validator is only coercing some of the booleans...

I think we may need to clean up our handling of booleans internally to try to normalize to true and false at the edges (controllers and backends). I don't think that's causing this problem, but it will still be helpful.

Does this additional commit make any sense to you? Because it doesn't to me. All I know is it passes the test and for that I'm okay with releasing it if you are.

@preaction

This comment has been minimized.

Copy link
Owner

preaction commented Jan 8, 2019

Also, we may need to add a specific Travis build with PERL5OPT=-MDevel::Hide=Cpanel::JSON::XS just to make sure that everything also works correctly with JSON::PP.

@mohawk2

This comment has been minimized.

Copy link
Contributor

mohawk2 commented Jan 8, 2019

The extra commit makes sense to me, and is similar to what I tried. Yes, agreed with the Devel::Hide thing. Are you happy to do that or should I?

@preaction

This comment has been minimized.

Copy link
Owner

preaction commented Jan 8, 2019

No, I'm wrong: We would need a test with Cpanel::JSON::XS, since Mojo::JSON prefers it in cases where it exists. The 5.14 test was failing without Cpanel::JSON::XS, which made it fall back to Mojo::JSON and JSON::PP::Boolean booleans... Either way, that's not important for this. Let's ship it!

@preaction preaction merged commit c20689b into preaction:master Jan 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@preaction

This comment has been minimized.

Copy link
Owner

preaction commented Jan 8, 2019

... and Mojo::JSON always uses JSON::PP::true and JSON::PP::false... Curiouser and curiouser...

preaction added a commit that referenced this pull request Jan 9, 2019

release v1.021
    [Added]

    - Added some more intelligent inferences about a configured OpenAPI
      spec to hook up the controller. In short: It's now easier to
      provide your own OpenAPI spec to work with the Yancy editor.
      Thanks @mohawk2 for continuing this work! [Github #43]

    [Fixed]

    - Fixed compatibility with JSON::Validator version 3. Thanks
      @mohawk2 for the patch! [Github #45] Thanks @eserte for the report
      [Github #44]

@mohawk2 mohawk2 deleted the mohawk2:fix-jv3 branch Jan 10, 2019

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