search

Yelp / bravado-core

Other
109 stars 98 forks source link

Replace __eq__ with is_equal to allow hashability #364

Closed macisamuele closed 4 years ago

macisamuele commented 4 years ago

This PR deprecates #363 As users of the library might be using hashability of bravado_core.spec.Spec and we don't have a strong need to have equality checks (other than for testing) I'm proposing to preserve the default equality checks and move the customly defined ones (PR #360) into a different method is_equal such that we can have an entry point of equality checks without altering the hashability properties of the objects.

NOTE: In #363 I was trying to define an __hash__ method were needed. Unfortunately defining hash functions is not a trivial task as it might be computationally expensive and grows with the size of the specs (which is usually when you'd like to use it even more).

From #363

The issue has been noticed because Yelp/swagger-spec-compatibility tests are broken (due to the fact that bravado_core.spec.Spec instances are not hashable).

In #360 equality methods have been added in order to increase testing coverage of deepcopy of instances.

Objects inheriting from object (aka all) have:

  • __eq__ implemented as always returning False
  • __hash__ implemented as returning a unique identifier of the specific instance (aka result of id)

Unfortunately, on Python, if you override one __eq__ method then the "default" __hash__ implementation is not is use, and so to make the object hashable again you need to implement __hash__ yourself. (Documentation reference)

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.2%) to 98.664% when pulling 4d30c0a7016f419138d15c644428412d493263a0 on macisamuele:maci-replace-eq-with-is_equal-to-allow-hashability into 9c20a251f016870f1f716b7217d3dae4ebbf9c56 on Yelp:master.

macisamuele commented 4 years ago

Test failures are addressed in https://github.com/Yelp/bravado-core/pull/365