bastikr / boolean.py

Implements boolean algebra in one module.
BSD 2-Clause "Simplified" License
78 stars 34 forks source link

Symbol.__lt__ assumes the class of some_symbol.obj implements __lt__ #75

Closed gemerden closed 7 years ago

gemerden commented 7 years ago

This is a small issue, i wouldn't say anything if it didn't happen to me ;-)

    def __lt__(self, other):
        comparator = Expression.__lt__(self, other)
        if comparator is not NotImplemented:
            return comparator
        if isinstance(other, Symbol):
            return self.obj < other.obj   # <= this might raise exception
        return NotImplemented

could be more robust (probably, can't oversee all the consequences), with:

    def __lt__(self, other):
        comparator = Expression.__lt__(self, other)
        if comparator is not NotImplemented:
            return comparator
        if isinstance(other, Symbol):
            try:
                return self.obj < other.obj
            except Exception:
                return NotImplemented
        return NotImplemented

This is called in simplify. For now i will override Symbol.lt in my code.

Cheers, Lars

pombredanne commented 7 years ago

Looking good. Care for sending a PR may be with a small test case?

gemerden commented 7 years ago

Sure, but i have no experience with PR's. I think i need to be a collaborator to publish a branch, can you make me one?

gemerden commented 7 years ago

i tried some more and standard python types without lt somewhere are hard to find, also it was complex to write a good test (since i think at least some python versions fall back to not __gt__ or some such). Due to rarity of problem and it was taking me too much time, i withdraw the issue ...