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

Clean up name API #1

Merged
merged 7 commits into from
Sep 20, 2023
Merged

Conversation

djc
Copy link

@djc djc commented Sep 20, 2023

This removes the From impls and that seems to have some interesting downstream effects in terms of making types/methods unused (I guess because the From impls make the types constructable from a foreign caller?).

Just layering on top of your PR probably doesn't exactly make sense -- maybe you can look at what got added for the dns_names() API originally and how much that we could/should revert with these changes?

The From impl feels a little unidiomatic because the DnsNameRef is not
consumed. An AsRef impl would unnecessarily constrain the lifetime of
the output value to `&self`, whereas it can live as long as `'a`.
The From impl feels a little unidiomatic because the WildcardDnsNameRef is
not consumed. An AsRef impl would unnecessarily constrain the lifetime of
the output value to `&self`, whereas it can live as long as `'a`.
The From impl feels a little unidiomatic because the GeneralDnsNameRef is
not consumed. An AsRef impl would unnecessarily constrain the lifetime of
the output value to `&self`, whereas it can live as long as `'a`.
Copy link
Owner

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks!

@cpu cpu merged commit 183a10b into cpu:cpu-rework-dns-names-iter Sep 20, 2023
22 of 23 checks passed
@cpu cpu deleted the clean-up-name-api branch September 20, 2023 14:01
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.

2 participants