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

Replace callbacks with futures #208

Closed
wants to merge 2 commits into from
Closed

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Apr 19, 2023

Motivation:

Even after the recent change (#207) to offer a future-based API, the mail client internals remained heavily callback-oriented, with some idiosyncratic quirks.

This PR changes most of the implementation to be future-based, which allows a lot of code cleanup. I wasn't aware of #194, so there's a lot of overlap. The SMTPConnection class still holds the 2 handlers and further refactoring would be great, I just couldn't figure it out myself. I'll see if I can find some inspiration in #194.

This PR has 2 commits (for now), the 1st mostly avoids touching SMTPConnection and focuses on everything around it, while the 2nd is focused on the SMTPConnection class. I figured this split might be helpful in reviewing (plus it reflects the order of my work), but I can squash the commits if you wish.

@Ladicek Ladicek requested review from vietj and gaol April 19, 2023 15:17
@Ladicek Ladicek changed the title Futurization Replace callbacks with futures Apr 19, 2023
@vietj vietj added this to the 5.0.0 milestone Apr 20, 2023
@gaol
Copy link
Member

gaol commented Apr 21, 2023

Thanks @Ladicek , yes, the internal did have lots of callbacks from old design.

The PR #194 tries to do part of the clean up focusing on merging 2 handlers into 1.

There are many states in SMTPConnection to take care of:

  • if the connection is closed by accident(or network) or intentionally
  • if the connection is in active use
  • if the connection pool tries to close the connection (see SMTPConnectionPoolShutdownTest)
  • if QUIT has been sent for a specific use case( quitCloseConnection does not close connection #175)

most of the states have been there when I started working on this project, I don't see any related issue report about it yet, so I followed what it was designed.

As you said, this PR has lots of conflicts with #194 (several months ago), do you mind we can have #194 merged first ? :)

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 27, 2023

I'm closing this until we figure out #194. I'll open a new PR then.

@Ladicek Ladicek closed this Apr 27, 2023
@Ladicek Ladicek deleted the futurization branch April 27, 2023 15:21
@Ladicek Ladicek removed this from the 5.0.0 milestone Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants