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

Add as_str to DNSName #129

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

Add as_str to DNSName #129

wants to merge 1 commit into from

Conversation

Rantanen
Copy link

There already exists AsRef<str> implementation for DNSName, but given there is also as_ref() -> DNSNameRef, trying to get a &str with dns_name.as_ref() ends up picking the wrong implementation.

The AsRef::<str>::as_ref(&source) alternative works, but isn't that pretty!

The as_str() is somewhat idiomatic solution to this (see String::as_str() for an example).

(The AsRef::<str>::... is a suitable workaround, which I've already unleashed upon my codebase so I don't really need this PR to land if there's a reason to reject it.)

Pull requests must explicitly indicate who owns the copyright to the code being contributed and that the code is being licensed under the same terms as the existing webpki code.

I don't believe the changes are significant enough to be covered by copyright, but to avoid any doubt, if they were, I'd be the owner and I'll license them under the same terms as the exiting webpki code.

@briansmith
Copy link
Owner

Sorry for the slow response. I agree that the situation with AsRef is unfortunate. I'll be updating PR #125 soon to try to merge DnsName and DnsNameRef into one type. As part of that, we should try to solve the AsRef problem.

Base automatically changed from master to main January 14, 2021 01:31
@briansmith
Copy link
Owner

Note: I renamed the "master" branch to "main". Sorry for the inconvenience. This PR has had its base branch updated to "main" but you'll need to deal with the change in your local repo yourself.

@Rantanen
Copy link
Author

This is a one line change (plus the comment), it might be better to just implement the change directly in #125 if there's a larger rework. :)

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