7
\$\begingroup\$

I have the following implementation for the __lt__. Could you please check if it's okay to compare objects like that?

class Record(NamedTuple):
    video_id: str
    uuid: Optional[UUID]
    mac: Optional[str]
    s3_bucket: str
    s3_key: str
    reference_date: date

    @staticmethod
    def _lt(a: Any, b: Any) -> bool:
        if a and b:
            return a < b
        if a is None and b:
            return True
        return False

    def __lt__(self, other: "Record") -> Union[bool, Type["NotImplementedType"]]:
        if not isinstance(other, Record):
            return NotImplemented
        for field in self._fields:
            self_ = getattr(self, field)
            other_ = getattr(other, field)
            if self_ == other_:
                continue
            return self._lt(self_, other_)
        return False
\$\endgroup\$
8
\$\begingroup\$

This code is dangerous:

        if a and b:
            return a < b
        if a is None and b:
            return True
        return False

Consider a = -1 and b = 0. a and b is False because b is falsey, so a < b is never computed.

Since a is None is false, we skip to return False, yielding a surprising result for Record._lt(-1, 0)

You should explicitly test a is not None and b is not None instead of a and b.

Based on your typing, it currently doesn’t look like you’d pass in an int or a float, but if the class changes in the future, or is used as the model for another similar class, the unexpected behaviour might arise.

\$\endgroup\$
2
  • \$\begingroup\$ what do you think about using @dataclass(order=True) instead of custom lt method? \$\endgroup\$ – Ivan Petrushenko Jun 17 at 8:29
  • \$\begingroup\$ A @dataclass is mutable (assuming frozen=True is not specified), where as tuples, including NamedTuple, are immutable. Without knowing more about the class and it's intended usages, it is hard to give a definitive answer. I like both @dataclass and NamedTuple, but they are different animals used for different things. \$\endgroup\$ – AJNeufeld Jun 21 at 19:21
4
\$\begingroup\$
  1. No code is always more correct than yes code. NamedTuple has comparison.

Unless one has a good reason to surprise the user with other than the perfectly good built-in behavior, in which case they would document that reason clearly, this is far from OK.

  1. You may want to use a dataclass instead, which supports type annotations better:
from dataclasses import dataclass

@dataclass(order=True)
class Record:
    video_id: str
    uuid: Optional[UUID]
    mac: Optional[str]
    s3_bucket: str
    s3_key: str
    reference_date: date

-- EDIT --

It's always good to describe the intent in code, by means of a (unit-) test script.

An example:

assert Record("myvid", None, None) < Record("yourvid", None, None)
assert Record("id", s3_key="a/b/c") < Record("id", s3_key="a/b/x")
try:
  Record("id") < Record(None)
else:
  raise AssertionError("should have raised")
...
\$\endgroup\$
5
  • \$\begingroup\$ Does default comparison works the same as mine? \$\endgroup\$ – Ivan Petrushenko Jun 17 at 8:28
  • \$\begingroup\$ for example, how to compare int and None? \$\endgroup\$ – Ivan Petrushenko Jun 17 at 8:46
  • \$\begingroup\$ I advise to write a simple test script to clarify your intent. I'll edit a hint to that in my answer. \$\endgroup\$ – xtofl Jun 17 at 8:50
  • \$\begingroup\$ so it's necessary to write a custom comparator because the exception will be thrown and we don't know what to return true or false \$\endgroup\$ – Ivan Petrushenko Jun 17 at 9:32
  • \$\begingroup\$ Wait - do you need to have a 'NotImplemented' as result (as in if compare(a, b) == NotImplemented)? Then don't hijack the __lt__, but define a dedicated function for this. Else users will definitely be surprised by this unexpected behavior. If you need the a<b to raise, you're good to go with the default implementation. \$\endgroup\$ – xtofl Jun 17 at 10:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.