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

Allow setting default Resolv::DNS config in Resolv.new #43

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

Conversation

jeremyevans
Copy link
Contributor

Instead of just passing the use_ipv6 option, pass all options given. By using a singular hash argument instead of keywords, this avoids the issue when a user does:

Resolv.new([Resolv::DNS.new], use_ipv6: false)

Which would have resulted in the use_ipv6 option being ignored.

This is backwards compatible, because there has not yet been a release with the use_ipv6 keyword supported.

Prompted by #14 (comment)

Copy link
Member

@sorah sorah left a comment

Choose a reason for hiding this comment

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

Wow, sorry I didn't notice this in time

since Ruby 3.3 was released on December so we need to keep use_ipv6: now…

@jeremyevans
Copy link
Contributor Author

My recommendation would be to warn for the :use_ipv6 option in Ruby 3.4, since it was just introduced in Ruby 3.3, and remove it in Ruby 3.5. Would you be OK with that?

@sorah
Copy link
Member

sorah commented Mar 5, 2024

sounds good for me

If a positional argument is provided, the use_ipv6 keyword
option is ignored:

```ruby
Resolv.new([Resolv::DNS.new], use_ipv6: false)
```

Issue a warning in this case.

Currently, you have to pass a positional hash argument to set
the DNS config.  However, after support for the use_ipv6 keyword
argument is removed, you will be able to pass either a positional
hash or keyword arguments.  Code that was just specifying the
use_ipv6 keyword optional will still work correctly in that case.
@jeremyevans
Copy link
Contributor Author

@sorah OK, I made that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants