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

Make exceptions hashable (but not comparable) #502

Merged
merged 1 commit into from
Dec 15, 2018

Conversation

stuarteberg
Copy link
Contributor

@stuarteberg stuarteberg commented Dec 6, 2018

This PR addresses #477.

Currently, ValidationError and SchemaError implement __eq__, but don't implement __hash__, which makes then unhashable. That prevents them from being used in certain scenarios, notably in a test suite that uses pytest [Edit: It fails under python 3.6, but not 3.7].

The easiest way to make them hashable is to simply remove the __eq__ implementation. I doubt many users of jsonschema rely on the fact that those errors are comparable.

The only problem is jsonschema's own test suite, which does rely on ValidationError.__eq__(). But that's easy to change, as I've done in this PR. Thoughts?

@codecov
Copy link

codecov bot commented Dec 6, 2018

Codecov Report

Merging #502 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #502      +/-   ##
=========================================
+ Coverage   94.89%   95.1%   +0.21%     
=========================================
  Files          19      19              
  Lines        2310    2309       -1     
  Branches      302     300       -2     
=========================================
+ Hits         2192    2196       +4     
+ Misses        104     100       -4     
+ Partials       14      13       -1

@Julian
Copy link
Member

Julian commented Dec 15, 2018

So, I'd actually like to move in the reverse direction longer term (where users can rely on comparisons, which is what they should do, a large reason why I added this originally was that there were many internal tests that were having to make many assertions instead of just one single comparison).

But! In the short term, I think this does make sense until we do the deprecations and can therefore roll this out more sanely.

So overall, thanks :), looks reasonable.

@Julian Julian merged commit 6f10ce6 into python-jsonschema:master Dec 15, 2018
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.

2 participants