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 _dict_from_slots, solves Path comparison #480

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

artemisart
Copy link

@artemisart artemisart commented Sep 3, 2024

Hello, I figured out some __slots__ could be not set which makes getattr fails and so DeepDiff._dict_from_slots fails, which is what makes Path comparison impossible currently:

from pathlib import Path

obj = Path('/test/yo')
keys = [key for cls in obj.__class__.__mro__ for key in getattr(cls, '__slots__', ())]
print(keys)
{k: getattr(obj, k, 'not present') for k in keys}
# gives:
['_drv', '_root', '_parts', '_str', '_hash', '_pparts', '_cached_cparts']
{'_drv': '',
 '_root': '/',
 '_parts': ['/', 'test', 'yo'],
 '_str': 'not present',
 '_hash': 'not present',
 '_pparts': 'not present',
 '_cached_cparts': 'not present'}

I fixed this with a hasattr in _dict_from_slots.

Impact:

from deepdiff import DeepDiff
from pathlib import Path

DeepDiff(Path('a'), Path('b/b'))
# Before this change
# {'unprocessed': ['root: a and b/b']}
# After this change
# {'values_changed': {'root._parts[0]': {'new_value': 'b', 'old_value': 'a'}}, 'iterable_item_added': {'root._parts[1]': 'b'}}

Solves #231

@seperman seperman changed the base branch from master to dev September 11, 2024 05:34
@seperman
Copy link
Owner

seperman commented Sep 11, 2024

@artemisart Thanks for making a PR!
Interesting, so far I have only used os.path. I learned something today.
Can you please fix the failing test test_bad_attribute and add your name and info to the bottom of AUTHORS.md please?

@artemisart
Copy link
Author

@seperman fixed!
I'm not sure what was expected for this test, hasattr(Bad(), 'x') returns False so now it doesn't fail anymore, is this ok?

Copy link
Owner

@seperman seperman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.55%. Comparing base (6d8a4c7) to head (ce1c8fb).
Report is 6 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #480      +/-   ##
==========================================
- Coverage   96.70%   96.55%   -0.16%     
==========================================
  Files          14       14              
  Lines        3946     3946              
==========================================
- Hits         3816     3810       -6     
- Misses        130      136       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seperman
Copy link
Owner

@artemisart We iterate through slot values to compare objects. Before your fix, we would have not processed the Bad() object. Now we process them. Not a super useful test after your fix, I agree.

@seperman seperman merged commit a114ed2 into seperman:dev Sep 12, 2024
6 of 7 checks passed
@artemisart artemisart deleted the patch-1 branch September 12, 2024 22:05
@artemisart
Copy link
Author

Note: you can close #231

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