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

RUST-911: Add srvServiceName URI option #1235

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jadalilleboe
Copy link
Contributor

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

looking good so far - we'll also need to implement the prose test described here

src/srv.rs Outdated
@@ -90,14 +94,21 @@ pub(crate) enum DomainMismatch {
#[cfg(feature = "dns-resolver")]
pub(crate) struct SrvResolver {
resolver: crate::runtime::AsyncResolver,
client_options: Option<ClientOptions>,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest only storing the service name rather than the entire options struct - it looks like that's the only field being accessed from this

@isabelatkinson
Copy link
Contributor

It looks like we're also currently skipping some URI options tests for srvServiceName that we should unskip as part of this. See here and here. (I think the comment on the second one points to the wrong ticket.)

src/srv.rs Outdated
.unwrap_or(default_service_name),
};
let lookup_hostname = format!("_{}._tcp.{}", service_name, original_hostname);
.unwrap_or("mongodb".to_string()),
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny style nit: we can do the following to avoid allocating an extra string here

self.srv_service_name
    .as_deref() // converts into an Option<&str> to get a reference to the inner string
    .unwrap_or("mongodb")

@isabelatkinson
Copy link
Contributor

disregard my comment about assertions, I misread which method was being called!

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

lgtm!

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