-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
Remove :CHECK-DNS-P and DNS lookups (pending) #3383
Conversation
…LID-TLD-P. 1) Replace (and check-dns-p (valid-tld-p ...)) with (valid-tld-p ...). I cannot remember why I added that extra conditional. 2) On the same line: replace valid-tld-p with valid-url-p. What if my query is "example.com/foo" rather than "example.com"? 3) Now check-dns-p is not needed anywhere in buffer.lisp, so I remove it. This fixes the situation when I want to visit "github.com/atlas-engineer" and get a search with the default search engine instead. Related commits: 377e9ab: makes :CHECK-DNS-P redundant, replaces a correct call to VALID-URL-P with a wrong call to VALID-TLD-P.
Also isn't it a good idea to remove dns lookups completely? |
@shamazmazum, in commit 377e9ab I had to preserve the API so that the fix would land in the 3-series releases. I had the same urge to remove DNS lookups as you. This is a good opportunity to change the API so thank you for bringing it up. Remove DNS lookupsFrom my cursory investigation, we brought them in commit fc25179. There must be a better way to solve the problem. If we remove Implicit bug report
This is a major bug, and it's my fault that I didn't take it into account. We need a fix that is compatible with the 3-series API. It may be slightly off-topic for this PR so I'd suggest opening an issue about it or sending a patch. Other quirksPerhaps somehow related to DNS lookups, there is also another less important issue that would be nice to fix. |
@aadcg I don't quite get what you mean by "a fix that is compatible with the 3-series API". Do you mean that some third-party software is supposed to call If you wish a smaller fix which just fixes a navigation to URLs like "example.com/foo", I can just fix an erroneous conditional in So what should I do? |
Technically, you're correct, but I'd rather lean on the side of being conservative and avoid changes to the function signature on releases that don't bump the major version.
In this PR, let's address removing DNS lookups, and we can make arbitrary changes in the codebase. In the separate one, let's do exactly what you suggest:
I'd suggest doing the small fix above first, and then working on this PR that gets rid of DNS lookups. Does that make sense? |
Yes, I've opened a new PR which fixes just the navigation. |
Done, see #3385 (comment). |
Hello! In the commit 377e9ab a call to
valid-url-p
was replaced withvalid-tld-p
in theinitialize-instance
fornew-url-query
which means that queries likegithub.com/atlas-engineer
are always opened in a search engine ((valid-tld-p "github.com/atals-engineer")
isnil
). After that commitcheck-dns-p
is also never passed tovalid-url-p
, so I remove it.The only place where
check-dns-p
is used is ininitialize-instance
itself. This argument was introduced by me in 485f042 to deal with long latency times when usingset-url
. I cannot remember why I added a check forcheck-dns-p
ininitialize-instance
and now I remove it in this commit.