WieeRd / ClockBot

Most interesting yet least useful bot on Discord
Mozilla Public License 2.0
6 stars 1 forks source link

feat: add `SendExt` to reduce boilerplate of common `send()` calls #4

Open WieeRd opened 10 months ago

WieeRd commented 10 months ago

SendExt Mixin

Provides convenience function to send embed with predefined color palette and icon.

WieeRd commented 10 months ago

No Traits?

In Rust, we have this cool thing called trait. With it, cool things like trait bounds and blanket implementation are possible.

// discord.abc.Messageable
trait Messageable {
    fn send(...) -> Message;
}

trait SendExt: Messageable {
    fn send_info(...) -> Message { self.send(...) }
    fn send_warn(...) -> Message { self.send(...) }
    fn send_error(...) -> Message { self.send(...) }
}

impl<T: Messageable> SendExt for T {}

// now we can use `send_{info, warn, error}()` on every Messageable
user.send_info("Prepare thyself")
context.send_warn("Thy end is now")
textchannel.send_error("Judgement")

Now, how am I supposed to do something similar to this in Python, using multi-inheritance (smh) instead of traits?

WieeRd commented 10 months ago

Coping Mechanism Candidates

Trait at home:

Protocol works like a trait with only required methods. Typical Mixins are like a trait with provided methods only.

A mixin class with @abstractmethod and concrete (implemented) methods might be the closest I can get to trait, although with greatly reduced flexibility.

Edit: Protocol can be used with @abstractmethod, and it can be explicitly inherited to provide default implementation. This seems to give the most breathing room since it's somewhat free from the inheritance hierarchy.

WieeRd commented 10 months ago

The Seething Process

Python is not Rust. Pretending as if it is or "hacking" it with things like PyTrait isn't going to help my experience and ruin the consistency with the rest of the language ecosystem.

That said, making .send_*() available to all subclasses of discord.abc.Messageable is most certainly impossible in today's Python (Messageable.send_err = ... is possible, but makes the type checker furious).

As a compromise, I'm going to create a custom context class which subclasses SendExt mixin.

WieeRd commented 10 months ago

Malding Implementation

Question: How do I put trait bounds on a mixin class?

In other words, if a mixin depends on method(s) of the mixed-into class, how can I ensure that the mixed-into class implements the required method?

SendExt and its methods, .send_*() are wrappers around discord.abc.Messageable.send(). So any class that wants to mixin SendExt must implement .send() by inheriting Messageable. This is like putting a trait bound on a trait (e.g. trait SendExt: Messageable {})

Can I enforce this with Python's type hints and static type checkers?

WieeRd commented 10 months ago

Attempt 1

from abc import ABC, abstractmethod

# discord.abc.Messageable
class Messageable:
    def send(self, content: str) -> None:
        print(content)

# commands.Context
class Context(Messageable):
    pass

class SendExt(ABC):
    @abstractmethod
    def send(self, content: str) -> None: ...

    def send_warn(self, content: str) -> None:
        self.send(f"WARN: {content}")

# custom `Context` to inject `send_*()`
class MacLak(Context, SendExt):
    pass

ctx = MacLak()
ctx.send_warn("I am inside your walls")

As seen in the unresolved SSO question, one way is to make SendExt a ABC. With Messageable.send()'s signature redefined as @abstractmethod, it becomes mandatory for SendExt's subclass to implement .send().

# try to mixin `SendExt` without implementing `.send()`
class Nope(SendExt):
    pass

nope = Nope()  # fails; cannot be instantiated without overriding all @abstractmethod

Problems

# can `.send()` to DM
class User(Messageable):
    pass

steven = User()

# incompatible type; can be used with duck typing but Pyright is still mad
SendExt.send_warn(steven, "Only language you speak is FAILURE")
WieeRd commented 10 months ago

Attempt 2

Type hinting the self parameter.

class SendExt(Protocol):
    def send_warn(self: Messageable, content: str) -> None:
        self.send(f"WARN: {content}")

The self parameter of the Protocol's method does not have to be Self. By adding : Messageable to each .send_*() method, re-defining send() becomes unnecessary. This solves problem 1 and 2 from attempt 1. However, the 3rd problem still remains.

WieeRd commented 10 months ago

Attempt 3

from abc import abstractmethod
from typing import Protocol

# discord.abc.Messageable
class Messageable(Protocol):
    def send(self, content: str) -> None:
        print(content)

    @abstractmethod
    def _get_channel(self, ident: int) -> None: ...

# commands.Context
class Context(Messageable):
    def _get_channel(self, ident: int) -> None:
        print(f"Context: {ident}")

# send_ext.py
class SendExt(Messageable, Protocol):
    def send_warn(self: Messageable, content: str) -> None:
        self.send(f"WARN: {content}")

This allows us to:

# custom `Context` to inject `send_*()`
class MacLak(Context, SendExt):
    pass

ctx = MacLak()
ctx.send_warn("Directly available as a method")
class NotMessageable(SendExt):
    pass

# the `Messageable._get_channel` abstract method is not overriden
try:
    _ = NotMessageable()  # type: ignore[abstract]
except TypeError:
    print("Does not override `_get_channel()`, cannot be instantiated")
class User(Messageable):
    def _get_channel(self, ident: int) -> None:
        print(f"User: {ident}")

user = User()
SendExt.send_warn(user, "Still able to use `SendExt.*()` as a function")

Problem

There is a tiny problem. It turns out...

discord.abc.Messageable is actually neither ABC nor Protocol.

# current state (v2.3.2) of discord.py
class Messageable:
    def send(self, content: str) -> None:
        print(content)

    def _get_channel(self, ident: int) -> None:
        raise NotImplementedError

It's just a normal class, and the required method _get_channel() just raises error. WHY???

WieeRd commented 10 months ago

Investigation

I assume it was because most features of Messageable are defined in itself, and the author thought it was more of a mixin than a protocol. Indeed, it never needs to be a protocol in almost every case; ...except my oddly specific niche use case for static type hinting.

Removing the Protocol had practically no impact on the library when it happened, So I suppose adding it back will be just as harmless. I'll have to make a PR and somehow persuade Danny and the maintainers.

WieeRd commented 10 months ago

Lessons Learned

Here's an MRE version of what I struggled to achieve so far:

trait Foo {
    fn foo(&self);
}

trait Bar: Foo {
    fn bar(&self) {
        self.foo();
    }
}
from abc import abstractmethod
from typing import Protocol

class Fooable(Protocol):
    @abstractmethod
    def foo(self):
        ...

class BarMixin(Foo, Protocol):
    def bar(self: Foo):
        self.foo()

Combination of Protocol, @abstractmethod, and an explicit type hint on self. All of it, just to achieve : Foo "trait bound" on Python Mixin class.

Honestly, it's kind of depressing to see Rust/Python snippets right next to each other, and feel how much the amount of effort differs to express the intention with type.

Well, I could have just suppressed the warnings and get along with duck typing, but I was curious how much type expression in Python has advanced since 3.8. The conclusion is that I'll be using Protocols. A lot.