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

Use dataclass for email data #9365

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mtomilov
Copy link
Contributor

@mtomilov mtomilov commented Feb 20, 2025

Refs #9332

Now that we've added tag to email data it makes sense to incapsulate it all in a separate class and pass it around instead of a tuple.

Testing

To test support of both send signatures you can run something like this from the cli.

    email = EmailData(
        recipients=["example.com"],
        subject="My email subject",
        body="Some text body",
        tag=EmailTag.TEST,
        html="<p>An HTML body</p>",
    )
    mailer.send.delay(email)
    mailer.send.delay(recipients=email.recipients, subject=email.subject, body=email.body, tag=email.tag, html=email.html)

@mtomilov mtomilov force-pushed the use-dataclass-for-email-data branch from fdfecfa to 27dd03a Compare February 20, 2025 15:21

:param request: the current request
:type request: pyramid.request.Request
Copy link
Contributor Author

@mtomilov mtomilov Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the types to the signature of the function

tag: EmailTag
html: str | None = None

def to_message(self) -> pyramid_mailer.message.Message:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incapsulating converting the email data into pyramid mailer message

@mtomilov mtomilov force-pushed the use-dataclass-for-email-data branch from 27dd03a to 41bfb2c Compare February 20, 2025 17:45

@celery.task(bind=True, max_retries=3, acks_late=True)

@celery.task(bind=True, max_retries=3, acks_late=True, serializer="pickle")
Copy link
Contributor Author

@mtomilov mtomilov Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling pickle to support dataclass serialization just for this task

@@ -19,6 +19,7 @@

celery = Celery("h")
celery.conf.update(
accept_content=['json', 'pickle'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Letting the workers accept pickled dataclass serialization
Not sure what are policy here is but having to stick to json-serializable is kinda limiting.
Alternatively we could write our own serializer / deserializer pair and register it with kombu.serialization to be more explicit with what we send and receive.

@mtomilov mtomilov force-pushed the use-dataclass-for-email-data branch 2 times, most recently from 9701aae to 881700a Compare February 21, 2025 12:01
subject: str,
body: str,
tag: EmailTag,
recipients: list[str] | EmailData,
Copy link
Contributor Author

@mtomilov mtomilov Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number of arguments doesn't change but we support both old and new signatures by inspecting the type of the first argument below to support in-flight celery tasks with the old arguments.

@mtomilov mtomilov force-pushed the use-dataclass-for-email-data branch from 881700a to adb7578 Compare February 21, 2025 12:04
@mtomilov mtomilov requested a review from seanh February 21, 2025 12:07
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

Successfully merging this pull request may close these issues.

1 participant