search

tgockel / json-voorhees

A killer modern C++ library for interacting with JSON.
http://tgockel.github.io/json-voorhees/
Apache License 2.0
128 stars 18 forks source link

Modified check_type(...) to add "but found <type>" #121

Closed jjberry314 closed 5 years ago

jjberry314 commented 5 years ago

The overload for a single type adds "but found " the end of the string, but the overload for multiple types does not. Now it does.

jjberry314 commented 5 years ago

My proposed changes to empty() and size() need to be reviewed by Travis in case he is relying on the previous behaviour elsewhere in the library. I realise that there is a backward compatibility risk here, so I am happy for this change to be denied.

I think my change to check_type() is benign though.

jjberry314 commented 5 years ago

I think that, on reflection, perhaps size() == 0 is not valid for a value of type null, since its size is indeterminate rather than zero, strictly speaking.

I do still think that empty() should return true for a null type though.

tgockel commented 5 years ago

Definitely like the additional debug-ability here. Big question is if these changes should go into a 1.4 or 2.0, since there are potential breaking changes in behavior here. My gut says that something that would have thrown an error no longer doing so is only a minor version bump.

jjberry314 commented 5 years ago

The extra debug seemed useful to me, and also consistent with the other overload.

jjberry314 commented 5 years ago

As for the version number, I agree with you. Not throwing an exception when before it did is a minor change. It's unlikely that someone would be relying on that behaviour. I don't think it warrants a major version number change as it is not a major change in behaviour.

jjberry314 commented 5 years ago

I've made the change to empty(). The return value of default: is largely irrelevant as unless you add a new kind, it will never be hit. I've also modified the doxygen comments for it.

I noticed that size() is returning a boolean false implicitly cast to size_t as its default return value, and I know some compilers will issue a warning about this, so I changed it to a 0.