Skip to content

Issue 1487 - Suggested Fix - Return false when comparing Object against values that can't be unpacked #1488

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

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

alexgware
Copy link
Contributor

This is a suggested fix for Issue 1847

In cases where eq is used for an object that cannot be unpacked (i.e. is not an instance of Mapping) simply default to returning false.

Depending on style/preference, another way this could be implemented is:

try:
    return dict(**self) == dict(**other)
except TypeError:
    return False

I went with the Mapping approach as it seemed to tie in more nicely with the inheritance from MutableMapping for Object, but both are pythonic and I'm happy to switch the fix over if you have a preference.

I made the assumption that the decision to do equality explicitly against the unpacked versions of the objects was in part to maintain backwards compatibility with old style Fiona representations of geometries (or similar). So I tried to stay away from anything that would create more direct comparisons between object types (e.g. return false if it's not a dict/Object), leaving the option open for other Mapping based objects to also resolve to True when compared.

Alex Ware added 2 commits February 20, 2025 15:39
…e explicitly checked for equality, otherwise assume not equal
Copy link
Member

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

Thank you @agware ! This is right on the mark and very well done.

@sgillies sgillies added the bug label Feb 20, 2025
@sgillies sgillies self-assigned this Feb 20, 2025
@sgillies sgillies merged commit ec9768a into Toblerity:main Feb 20, 2025
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants