search

jhthorsen / json-validator

:cop: Validate data against a JSON schema
https://metacpan.org/release/JSON-Validator
56 stars 58 forks source link

Update Changes to clarify format API change #142

Closed mohawk2 closed 5 years ago

mohawk2 commented 5 years ago

Summary

Changes is vague on "formats changes"

Motivation

It took me nearly 3 hours to figure out exactly how JV 3+ broke Yancy

References

https://github.com/preaction/Yancy/issues/44 https://github.com/preaction/Yancy/pull/45

jhthorsen commented 5 years ago

I'm not sure how helpful that is. I was hoping that actually documenting how formats works was enough: https://metacpan.org/pod/JSON::Validator#formats (It was never documented before)

Maybe a documentation update would be better? If not then I would rather rephrase Changes to something like:

-  - Changed how format validation is done
+  - Breaking change: format callbacks need to return undef on success and a description on error
+    https://github.com/mojolicious/json-validator/commit/6b5261dbe0026f297cabed7cbcf664b0dc5987b0
mohawk2 commented 5 years ago

That is also fine, as long as it's very explicit that it's a breaking change, and what the change is.

I think the "coerce" change should also be spelled out a bit more (as a breaking change) as well, as that was the largest set of errors in Yancy.

jhthorsen commented 5 years ago

I don't understand how improving coercion would break anything. Can you explain?

mohawk2 commented 5 years ago

Because it functioned differently before, and with the behaviour changing, that broke Yancy's tests. See the fixes https://github.com/preaction/Yancy/commit/c20689be757e7aa9dba8ca5bc0b657c4a449035c and https://github.com/preaction/Yancy/commit/67746d01a82b1e157f9985638b1bd1d9c7494f2e#diff-b1348b5d64065a81bc06caea2469534bL943