-
Notifications
You must be signed in to change notification settings - Fork 1k
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
refactor(dns): unify symbol naming #4505
Conversation
Thanks! To make this an easier transition for our users, we usually keep the old symbols but deprecate them and mention in the deprecation message, what they have been renamed to. You can do that by creating a type-alias with the old name in place of the old symbol! That would also make this a non-breaking change which means we can release it immediately and don't have to wait for the next breaking release. Can you add those types please? :) See https://github.com/libp2p/rust-libp2p/pull/3852/files#diff-8afb61a90391b0558bf7653cf3ddaba5dec55a51ddac2eeb1d499d9af84b6b7d for an example! |
A good way to test this is to temporarily undo all changes in other crates in the workspace. They should still compile after that! :) |
I made the following changes based on the reviews.
I also temporarily reverted all changes to other crates in the workspace and checked to see if the test passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I've left a few more comments :)
The version in this branch has been changed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes!
I am happy to delay the documentation update but it just occurred to me that we should actually name the main struct here Transport
to be consistent with others.
libp2p-tcp
for example has a Transport
AND a Config
so naming this one here Config
would be a bit confusing. Sorry for not noticing this earlier.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
refactor(dns): rename `Config` to `Transport` Co-authored-by: Thomas Eizinger <thomas@eizinger.io> update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! This is ready modulo a few errors in the documentation :)
See CI failures. |
Thanks for this work! :) |
Description
Renamed the following
dns::GenDnsConfig
->dns::Config
dns::DnsConfig
->dns::async_std::Config
dns::TokioDnsConfig
->dns::tokio::Config
If async-std feature is enable, use
dns::async_std::Config
. When using tokio, importdns::tokio::Config
. There is no need to usedns::Config
directly.Resolves #4486.
Related: #2217.
Links to any relevant issues
Tracking issue for renaming symbols across the repository #2217
Change checklist
I have added tests that prove my fix is effective or that my feature works