-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
support for search options in /etc/resolv.conf #199
base: 1.x
Are you sure you want to change the base?
Conversation
Thank you for filing this PR, could you include unit tests, and look into fixing the failing unit tests? |
so I have fixed unit tests, when will u accept? |
Hey @Chrisdowson, thanks for updating the PR 👍 Unfortunately the test are currently failing, could look into that. The reactphp/dns project has over 20 million installations and we ourselves use it in nearly every ReactPHP project. This is why we have to assure that everything works flawlessly to avoid breaking current installations. |
@SimonFrings ,Hi, I have fixed bugs, And the test are success,When will you accept it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chrisdowson Thanks for the update 👍
I added a few remarks down below.
$promise = $this->resolver->resolve('google.com'); | ||
$promise = $this->resolver->resolve('bing.com'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we should change google.com
to bing.com
here, seems unnecessary. Same goes for the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In China,we can not visit google.com, so tests will not pass on my computer,But bing.com we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you that we should use something that works for everyone, in some of our projects we started using reactphp.org
instead, but I don't think this belongs in this pull request. Maybe use bing.com
for testing purposes on your system without committing it in the end.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimonFrings IMHO it makes sense to change everything to reactphp.org
to align this across the board. Will file PoC PR for that to take this discussion there.
@Chrisdowson I assume you can reacht reactphp.org
just fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes,I can reach reactphp.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you that we should use something that works for everyone, in some of our projects we started using
reactphp.org
instead, but I don't think this belongs in this pull request. Maybe usebing.com
for testing purposes on your system without committing it in the end.What do you think?
I agree with WyriHaximus, How about I replace bing.com
with reactphp.org
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WyriHaximus @Chrisdowson I agree with both of you, I am not sure if this is something we should tackle in this pull request as it doesn't fit to the other changes made in here and could seem a bit random. It also keeps the diff significantly smaller and lays focus on the actual changes. That's the reason I suggest a (follow-up) pull request where we change google.com
to reactphp.org
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimonFrings @Chrisdowson just opened reactphp/event-loop#263 to kick off the discussion.
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a leftover.
em, long time now,When will this PR be merged.And what should I do? |
Hey @Chrisdowson, I just had a chat with @SimonFrings about your pull request, and he mentioned that there are still some suggested changes remaining after his initial review. Once those changes are made, your pull request will go through another round of review. Additionally we don't have to change google in our tests as this has already been done in https://github.com/reactphp/event-loop/pull/263/files. I hope this answer keeps you in the loop and we can move forward. 👍 |
No description provided.