Inverted attribute order

[jaraco]

In #36, we learned that there's an inverted attribute order in:

tempora/tempora/schedule.py

Line 121 in 6579e3c

def at_time(cls, at, delay, target):

@Avasam can you elaborate on the concern?

[Avasam]

Sure! This relates to the Liskov substitution principle

Take the following example:

class A:
    def method(self, message: str, count: int) -> None:
        print("Message:" + message, count + 1)


class B(A):
    def method(self, count: int, message: str) -> None:
        print("Subclass message:" + message, count + 1)


def do_something(a: A) -> None:
    if isinstance(a, A):
        a.method("High", 5)


# Passes type-checking and isinstance check
do_something(A())

# Passes type-checking and isinstance check, but raises TypeError
do_something(B())

It's even worst if the parameter types are all compatible, because the error might spread further in. In tempora's case, one might unknowingly flip delay or target. Although since the parameter count is different, you have a second error that will be hit first that mypy is also raising:

class DelayedCommand:
    @classmethod
    def at_time(cls, at, target): ...


class PeriodicCommandFixedDelay(DelayedCommand):
    @classmethod
    def at_time(cls, at, delay, target): ...


def do_something(command: DelayedCommand) -> None:
    if isinstance(command, DelayedCommand):
        command.at_time(now(), lambda x:x)


# Passes type-checking and isinstance check
do_something(DelayedCommand())

# Passes type-checking and isinstance check, but raises:
# TypeError: at_time() missing 1 required positional argument: 'target'
do_something(PeriodicCommandFixedDelay())

To further show how unexpected this error can be:

Which can be solved either by adding the unused parameter to the subclass; or making the additional parameter optional; or saying screw the LSP, users of this code should explicitly make sure they're not getting this subclass (maybe its usage is rare and specialized).

Even if you decide to violate the LSP by design, keeping the parameter order is nicer and more consistent when used:

at = now()
target = lambda x:x
delay = 5

# This
def do_something(command: DelayedCommand) -> None:
    if isinstance(command, PeriodicCommandFixedDelay):
        command.at_time(at, delay, target)
    else:
        command.at_time(at, target)

# instead of
def do_something(command: DelayedCommand) -> None:
    if isinstance(command, PeriodicCommandFixedDelay):
        command.at_time(at, target, delay)
    else:
        command.at_time(at, target)

[jaraco]

Thanks for the thorough explanation. Looking at these functions, I mean for them to be alternate constructors. I don't intend for them to be used as instance methods or for Liskov substitution to apply. I merely wish for PeriodicCommand and DelayedCommand to be constructable from a specific time and PeriodicCommandFixedDelay to be constructed from a time and delay. I would not expect a user to have a DelayedCommand class or subclass and be able to construct an instance without knowing which subclass.

The reason I put target at the end was because I want the caller to focus first on "when" (at, maybe delay) and then "what" (target). Same for DelayedCommand.after. Putting target between at and delay is awkward from that vantage.

Perhaps there's a better way, perhaps to have a When class that encapsulates "at, maybe delay", and then every class could have a consistent interface. Or maybe we could make target keyword-only. Or perhaps we factor out target as a parameter altogether and add a do helper:

cmd = DelayedCommand.after(5).do(the_thing)

i.e.

class DelayedCommand(datetime.datetime):
    @classmethod
    def after(cls, delay) -> Self:
        ...

    def do(self, target) -> Self:
        self.target = target
        return self

That still doesn't satisfy LSP, however, as PeriodicCommandFixedDelay still requires different inputs to construct.

In short, because these are constructors and not meant to be used as instance methods, LSP doesn't apply.

[Avasam]

In that case, a suppression comment linking back here should be fine :)