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

feat: create dns.NewStreamDialer #180

Merged
merged 3 commits into from
Feb 16, 2024
Merged

feat: create dns.NewStreamDialer #180

merged 3 commits into from
Feb 16, 2024

Conversation

fortuna
Copy link
Contributor

@fortuna fortuna commented Feb 15, 2024

This allows people to create dialers from Resolvers and actually use the dns library.

@fortuna fortuna requested a review from jyyi1 February 15, 2024 22:27
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Code LGTM, with some small suggestions. Also please update the PR title to "feat: create dns.NewStreamDialer", GitHub supports markdown syntax in commit messages now.


// NewStreamDialer creates a [transport.StreamDialer] that uses Happy Eyeballs v2 to establish a connection.
// It uses resolver to map host names to IP addresses, and the given dialer to attempt connections.
func NewStreamDialer(resolver Resolver, dialer transport.StreamDialer) transport.StreamDialer {
Copy link
Contributor

@jyyi1 jyyi1 Feb 16, 2024

Choose a reason for hiding this comment

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

Shall we use (transport.StreamDialer, error) as the return type to align with other functions (also in case in the future we might return errors, so the caller don't need to change the code)? Even in this function, we can return errors if either resolver or dialer is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I've updated as per your suggestion

@fortuna fortuna changed the title Create dns.NewStreamDialer feat: create dns.NewStreamDialer Feb 16, 2024
@fortuna fortuna requested a review from jyyi1 February 16, 2024 20:45
@fortuna fortuna merged commit f741c57 into main Feb 16, 2024
6 checks passed
@fortuna fortuna deleted the fortuna-resdial branch February 21, 2024 03:57
fortuna added a commit that referenced this pull request Apr 26, 2024
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