Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance equality checks #371

Merged
merged 10 commits into from
Feb 24, 2020

Conversation

macisamuele
Copy link
Collaborator

While working on a branch related to #370 I've noticed that the implemented equality checks are good but not enough ;)

Currently the checks are making some assumptions on the state of certain attributes:

  • "It is built via Spec.build so we're ignoring it", but it is on the public interface so users might just modify it
  • fully skipping the check around resources and operations due to recursive Spec references
  • not considering that in the case of internally_dereference_refs: true the check (as it was implemented) would have had an unbounded recursion

The goal of this PR is to address those issues (still the checks are not perfect, but they should be more accurate) as well as providing a sort of method documentation.

…es to be private to prevent external modification.

The properties are still exposed as property to not break API compatibility and to preserve access to the features
…s (makes pickling and equality checks more complicated)
Update is_equal of Resource and Operation to allow ignoring swagger_spec attribute
@coveralls
Copy link

coveralls commented Feb 23, 2020

Coverage Status

Coverage increased (+0.5%) to 99.006% when pulling 7701eb2 on macisamuele:maci-enhance-equality-checks into 7684799 on Yelp:master.

@macisamuele macisamuele merged commit cfe47aa into Yelp:master Feb 24, 2020
@macisamuele macisamuele deleted the maci-enhance-equality-checks branch February 24, 2020 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants