Skip to content

fix exception handling in attribute comparison #61

Closed
wants to merge 2 commits into from

2 participants

@underrun
underrun commented Jul 5, 2012

Handle dumb comparisons between _Attrib and non-dict-like objects.
Return false for comparisons to objects that fail convert to dict.

There may be a better/faster/more efficient way to do this in cython (not so familiar with it), but this fixed my issue. I thought about testing the 'other' object for dict-likeness, but it isn't super easy as it not only has to be iterable but an iterable of iterables of length 2 unless it's already a dict or dict subclass... EAFP right?

dwilson fix exception handling in attribute comparison
Handle dumb comparisons between _Attrib and non-dict-like objects.
Return false for comparisons to objects that fail convert to dict.
843cb52
@scoder
lxml member
scoder commented on 843cb52 Jul 5, 2012

Note that "one" is not "self". Rich comparison may be called with arbitrary objects for both.

Also note that rich comparison is called for any comparison operator, i.e. <, <=, ==, =>, >, !=. It doesn't make sense to always return False on failure.

@scoder how can "one" not be "self" if __richcmp__ is not a staticmethod? some cython magic I've yet to learn?

Heh, you are right that it doesn't make sense to return false especially for both == and != ... sorry about that.

So, why is the dict conversion needed at all, shouldn't rich compare handle equivalence of objects with different types itself? If we do need the dict conversion, then the function should look something like:

def __richcmp__(one, other, int op):
    if not isinstance(one, dict):
        try:
            one = dict(one)
        except (TypeError, ValueError):
            pass
    if not isinstance(other, dict):
        try:
            other = dict(other)
        except (TypeError, ValueError):
            pass
    return python.PyObject_RichCompare(one, other, op)
lxml member
dwilson pass on dict conversion fail in _Attrib richcmp
to make sure richcmp still makes sense with both == and !=, don't
just return False on dict conversion failure in _Attrib __richcmp__.
instead, try conversion of both sides to dict and just pass objects on
for comparison as is if the conversion fails
3882d24
@underrun
underrun commented Jul 6, 2012

this seems to make more sense. I mean, in the sense that python returns a result for [] < {} or [[]] > None but at least we get different results for == vs != and > vs < now.

@scoder
lxml member
scoder commented on 3882d24 Aug 12, 2012

And when both conversions fail, you enter an endless loop.

lxml member

Hmm, sorry - actually, at least one of them will succeed, because one of them is an _Attrib object.

@scoder
lxml member
scoder commented Aug 14, 2012

Rethinking this some more - is there any reason we're not just returning NotImplemented when any of the conversions fails?

@scoder
lxml member
scoder commented Aug 14, 2012

I've implemented that here:

9c4dcdc

@scoder scoder closed this Aug 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.