Bogdanp / dramatiq

A fast and reliable background task processing library for Python 3.
https://dramatiq.io
GNU Lesser General Public License v3.0
4.37k stars 313 forks source link

FeatReq: Typehints in dramatiq.middleware #521

Open dragonpaw opened 1 year ago

dragonpaw commented 1 year ago

Just a small request (doesn't everyone say that?). I love the ability to use my own middleware to customize the messages and add some custom logging. One thing I noticed is there's no type type hinting in the dramatiq.middleware.Middleware class. As a new developer to the package, for places where I am meant to extend the code, it's really helpful to have some hints as to the types to expect. Also might be good to note in the docs that you can alter the arguments, such as the message, and that will be propigated.

Simplified example of what I mean:

class LoggingMiddleware(dramatiq.middleware.Middleware):
    def after_enqueue(
        self,
        broker: dramatiq.broker.Broker,
        message: dramatiq.message.Message,
        delay: int | None,
    ):
        logger.debug("Queued job: %r", message)

It's a small thing sure, but it does make it easier to write middleware.

jenstroeger commented 1 year ago

Perhaps this discussion should be expanded to all of the Dramatic package: the current implementation doesn’t seem to use type hints (although I saw the odd use of it).

Perhaps, though, @Bogdanp plans to add them with v2 at some point?

dragonpaw commented 1 year ago

Definitely would be nice to do all of it. I asked specifically about the middleware as there's no documentation on what the arguments to the middleware functions actually are (just their names) and I need to write some to deal with Postgres schema changing while running jobs. So I had to do some custom logging to figure out what the arguments were to be able to make a middleware.

dragonpaw commented 1 year ago

A little bit of an update with the latest version just out:

We have a number of custom middlewares, and in some of them we will set message.failed = True in before_process_message, in order to have the message thrown in the dead letter queue for possible manual review. But the new dataclass of dramatiq.Message doesn't have this property. Technically I know it's actually using the MessageProxy,which does have failed, but I wasn't sure if that's the contract we should be writing to in the middlewares.