Yelp / bravado-core

Other
109 stars 98 forks source link

Remove unneded assertion #316

Closed sjaensch closed 5 years ago

sjaensch commented 5 years ago

We uncovered an issue with a test on Python 2.7 internally, which fails due to the error message being u'birth_date' is a required property' (with the leading u character), while the assertion didn't expect the u to be there.

Further debugging showed that this is not failing on travis due to us using simplejson, and its C module _speedups.so specifically. Using that we get str objects when loading JSON, while not using it (or using the stdlib json module) yields unicode strings and dictionary keys.

The Python 2 documentation for the json module clearly states that JSON strings should be decoded into Python unicode strings. So the simplejson C module changes behavior here.

It turns out we don't need that assertion anyway, so let's remove it altogether.

Side note: PyYAML returns str when decoding YAML on Python 2, so there's a difference in behavior for us on Python 2 depending on how the specs are written.

I've opted not to remove simplejson altogether since that could break a few things internally. The fact that we get str objects on Python 2 seems to match expectations, and on Python 3 it's not an issue anyway (we always get str, both for JSON and YAML, and that's what people expect).

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.1%) to 98.458% when pulling 703dbfa62fee8d5b48eb0464af64fac3e1235de8 on sjaensch:remove-unneeded-assertion into ab93f45caee8bf6a289403ac5099aa2930bab73a on Yelp:master.