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

Remove AsyncClientConnect and AsyncSecureClientConnect in favor of async constructors #1541

Merged
merged 3 commits into from
Aug 23, 2021

Conversation

ErwanDL
Copy link
Contributor

@ErwanDL ErwanDL commented Aug 20, 2021

This is the second PR associated with #1532, removing the intermediate AsyncClientConnect object in favor of async functions to construct the AsyncClient, as discussed on the issue.

On another note, I am quite confused by the AsyncClient vs AsyncClientConnect distinction. Are there reasons not to define AsyncClient::new and AsyncClient::connect as async fn that return an AsyncClient once they have been awaited ? This could remove the need for this intermediary AsyncConnect object.

I think those functions can be simplified. The only reason I can think that didn't happen is because AsyncClient is older than async/await in Rust. Most likely it can be simplified to be async fn instead of Future implementations. If that's something you'd be interested in working on, it would be appreciated...

I also removed AsyncSecureClientConnect and make similar changes to AsyncDnssecClient as they were also based on AsyncClientConnect/AsyncClient.

There should be very little to no changes in usage for users, and the tests still all pass on my machine without having had to make any change to them.

@bluejekyll
Copy link
Member

Thank you for these changes. I'll try to find some time tomorrow to review, but it might be another day or two after that.

@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #1541 (a953a52) into main (69829fa) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1541      +/-   ##
==========================================
- Coverage   83.41%   83.37%   -0.04%     
==========================================
  Files         170      171       +1     
  Lines       16848    16936      +88     
==========================================
+ Hits        14053    14120      +67     
- Misses       2795     2816      +21     

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

This looks great. I'm surprised no changes are necessary to tests, but that's a good thing. Means the API is pretty consistent.

Thank you for cleaning this up!

@bluejekyll bluejekyll merged commit 6d7dd11 into hickory-dns:main Aug 23, 2021
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.

None yet

3 participants