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 timeout option for MX DNS record lookup #8

Merged
merged 6 commits into from
Aug 2, 2024

Conversation

rkitover
Copy link
Contributor

Add optional second parameter opts, an object that can contain the properties timeout and checkMx. The checkMx property of the opts object overrides the checkMx function parameter. The timeout property, in ms module format, specifies the timeout for the DNS MX record lookup, the default is 10 seconds.

Add test, example and update the API documentation.

Add optional second parameter `opts`, an object that can contain the
properties `timeout` and `checkMx`. The `checkMx` property of the `opts`
object overrides the `checkMx` function parameter. The `timeout`
property, in `ms` module format, specifies the timeout for the DNS MX
record lookup, the default is 10 seconds.

Add test, example and update the API documentation.

Signed-off-by: Rafael Kitover <[email protected]>
@rkitover
Copy link
Contributor Author

rkitover commented Jul 1, 2024

@jesselpalmer can you take a look please?

@jesselpalmer
Copy link
Owner

Hi @rkitover! Thank you so much for the PR. I love the idea! I am trying to beef up the tests to ensure everything is covered. I'll update this thread when I'm done.

@rkitover
Copy link
Contributor Author

@jesselpalmer Hi, sorry to bother you, but any luck with your tests? I need this for a PR I'm doing for another node module.

@jesselpalmer
Copy link
Owner

Hey @rkitover, you definitely aren't bothering me! I appreciate the nudge 😄 This has been on my to-do list; I just haven't gotten around to it. I'll take a look this weekend.

@rkitover
Copy link
Contributor Author

Thank you!

@jesselpalmer
Copy link
Owner

@rkitover Ok, I created a PR for your PR 😅 :

rkitover#1

Let me know what you think!

Apologies for the delay. It's been a particularly busy last month 😊

@jesselpalmer jesselpalmer merged commit 5c08d6d into jesselpalmer:main Aug 2, 2024
0 of 3 checks passed
@rkitover
Copy link
Contributor Author

rkitover commented Aug 2, 2024

@jesselpalmer

Nice work, your node is much better than mine, I don't use it much.

I didn't know that you can assign variables in object form, that's very cool.

I would recommend strengthening your Git commit discipline.

This is my favorite general explanation of how to format Git commits:

. Every project has its own practice, so when you patch other projects you want to try to match them to some degree.

Some are very strict about having a commit for every specific change, others are fine with one or a few commits for a feature and related work.

There are a few different guidelines, find one that you like for your projects.

You also want to try to have a clean commit history, you can do this with:

git fetch --all --prune
git rebase -i origin/main

. This will open up your editor with instructions on how to edit/order your history for a branch.

Anyway this is all for future reference, it doesn't matter here.

I'd appreciate it if you can do a release, then I will rework my link-check PR.

@rkitover rkitover deleted the opts-timeout branch August 2, 2024 04:06
@jesselpalmer
Copy link
Owner

@rkitover Nice! Thank you so much for the resource and the tips!

Looks like I have a test acting up, so I'll fix that and then I'll do a release.

I'll update this thread after I put out the release.

@rkitover
Copy link
Contributor Author

rkitover commented Aug 2, 2024

Awesome, thank you!

@jesselpalmer
Copy link
Owner

@rkitover I just released 2.0.0, which included these changes!

@rkitover
Copy link
Contributor Author

Awesome, thank you!

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