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: add client to allow passing custom resolver #27

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

Conversation

babariviere
Copy link

Close #26

@@ -11,16 +11,27 @@ import (

const hexDigit = "0123456789abcdef"

var DefaultClient = &Client{resolver: net.DefaultResolver}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
var DefaultClient = &Client{resolver: net.DefaultResolver}
var DefaultDNSClient = &DNSClient{resolver: net.DefaultResolver}

Let's call it DNSClient for clarity with BulkClient.

var DefaultClient = &Client{resolver: net.DefaultResolver}

type Client struct {
resolver *net.Resolver
Copy link
Owner

Choose a reason for hiding this comment

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

I believe we should export resolver and then remove NewClient. The New abstraction is a bit premature.

Copy link
Author

Choose a reason for hiding this comment

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

If I do this, users of this lib will be able to create a Client without resolver, and this will lead to a crash.
So I would have to add a check on each function to ensure we have at least the default resolver.

Copy link
Owner

Choose a reason for hiding this comment

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

That panic still exists if you instantiated a DNSClient and ignored the New function.

If you're creating a DNSClient (or any foreign struct) you should probably glance at the fields.

Also, a New API here obscures the name of the fields (function calls are unkeyed) leading a more cognitive overhead when reviewing code, and makes the otherwise simple DNSClient type mysterious. My preference is to only introduce a New function when either:

  1. The struct has complex initialization logic
  2. Only some of the struct's fields are required (and it's not as simple as public = required, private = not)

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.

Allow providing a custom DNS resolver
2 participants