Add ShouldJSONEqual assertion

[philippgille]

This implements ShouldJSONEqual, which is one of the requested features in #633.

The naming can also be ShouldEqualJSON, but with ShouldTimeEqual already existing, the other one seems to be more consistent with that pattern.

[philippgille]

Sorry for the forced push, I forgot the commit sign-off again for the last two commits

[philippgille]

@yesnault is there room in v1.2.0 for this, or too late?

[yesnault]

@yesnault is there room in v1.2.0 for this, or too late?

Hi @philippgille , thank you for this PR, it's a great new feature.

No problem, it could be integrated into the 1.2.0 version.

[yesnault]

I think that the parser to detect the json could be more json compliant. There is a check for { or [, but a simple text or number is also a valid number, see all values on https://www.json.org/json-en.html

[philippgille]

You're right, an older spec restricted it to objects and arrays, but the latest spec doesn't: https://stackoverflow.com/questions/7487869/is-this-simple-string-considered-valid-json

It's a good point to be compliant to the latest spec, but I also wonder when someone writes an assertion for a string or bool, wouldn't it be preferable to use ShouldEqual "foo", ShouldBeTrue/ShouldBeFalse etc to be more precise? This could be documented to make clear.

What would you prefer:

1. Allow any of the JSON types here, leave users the choice between ShouldEqual/ShouldBeTrue etc and ShouldJSONEqual
2. Allow only objects and arrays and clearly document this in the README (the assertion list)
   • ⚠️ Which makes me realize I didn't add the new keyword to that list yet, I'll do that when you decided on one of the options
3. Change the name to even split between object/array to be precise, which also makes the implementation simpler / more concise: ShouldJSONObjectEqual + ShouldJSONArrayEqual
   • One benefit could be to even allow an actual list as param to check against, like ShouldJSONArrayEqual foo bar baz instead of ShouldJSONArrayEqual '["foo", "bar", "baz"]', although not sure if that brings much value

[yesnault]

I prefer the first proposal: let the choice to the user and having the ShouldJSONEqual supporting the different types. Maybe not the easiest to implement, but definitely the easiest for users to understand if we say that ShouldJSONEqual supports the full JSON spec.

[philippgille]

Makes sense 👍

Done in 5c05103

⚠️ JSON null are tricky, because Venom passes it as empty string and not Go's nil. Haven't checked if this is a fundamental limitation in the upstream callers in Venom, or if it can be changed. Do you know maybe?

And added to README in ba3769c

Looking at the assertion list in the README it's a bit messy, I think it would make sense to sort the assertions alphabetically. But then the ShouldJSONEqual feels misplaced. => WDYT about naming it ShouldEqualJSON like originally proposed in #633, and then for consistency create ShouldEqualTime as an alias for ShouldTimeEqual, which is required for backward compatibility. The latter could be marked as deprecated in the README.

[yesnault]

About naming, ShouldEqualJSON is ok too, perhaps we can add some alias in another pr than here if needed. Thank you for adding different json values.
LGTM