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

Fix equality check when other contains more keys #124

Merged
merged 3 commits into from
Aug 28, 2017

Conversation

akhomchenko
Copy link
Contributor

Test shows the issue.

Only Python version is affected.

P.S. I can not come up with more test cases, so I'm not sure that fix is final

@codecov
Copy link

codecov bot commented Aug 27, 2017

Codecov Report

Merging #124 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #124   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         300    302    +2     
=====================================
+ Hits          300    302    +2
Impacted Files Coverage Δ
multidict/_multidict_py.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd078b2...b947a0c. Read the comment docs.

@@ -120,6 +120,8 @@ def __eq__(self, other):
if i1 != i2 or v1 != v2:
return False
return True
if len(self) != len(other):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It must be possible to move this to before line 114 and remove check at line 117. They are equal, so no need to have it copy-pasted in two places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree.
Check from line 117 is faster than line 123: no python magic method __len__ is involved.
Code duplication is very low, let's merge PR as is.

Copy link
Member

@webknjaz webknjaz Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's about speed, then I'd convert len(self) into len(self._impl._items)

@asvetlov
Copy link
Member

done

@asvetlov asvetlov merged commit fff43c0 into aio-libs:master Aug 28, 2017
@akhomchenko akhomchenko deleted the fix/eq branch August 28, 2017 08:24
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