Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FeatReq: Typehints in dramatiq.middleware #521

Open
dragonpaw opened this issue Jan 5, 2023 · 3 comments
Open

FeatReq: Typehints in dramatiq.middleware #521

dragonpaw opened this issue Jan 5, 2023 · 3 comments

Comments

@dragonpaw
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants