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

Remove :CHECK-DNS-P and DNS lookups (pending) #3383

Closed
wants to merge 1 commit into from

Conversation

shamazmazum
Copy link
Contributor

Hello! In the commit 377e9ab a call to valid-url-p was replaced with valid-tld-p in the initialize-instance for new-url-query which means that queries like github.com/atlas-engineer are always opened in a search engine ((valid-tld-p "github.com/atals-engineer") is nil). After that commit check-dns-p is also never passed to valid-url-p, so I remove it.

The only place where check-dns-p is used is in initialize-instance itself. This argument was introduced by me in 485f042 to deal with long latency times when using set-url. I cannot remember why I added a check for check-dns-p in initialize-instance and now I remove it in this commit.

…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.
@shamazmazum
Copy link
Contributor Author

Also isn't it a good idea to remove dns lookups completely?

@aadcg
Copy link
Member

aadcg commented Apr 9, 2024

@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 lookups

From my cursory investigation, we brought them in commit fc25179. There must be a better way to solve the problem. If we remove check-dns-p from valid-url-p, some tests may fail so please check it. Still, if you can remove them and keep the functionality, I'm in strong favor.

Implicit bug report

queries like github.com/atlas-engineer are always opened in a search engine ((valid-tld-p "github.com/atals-engineer") is nil)

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 quirks

Perhaps somehow related to DNS lookups, there is also another less important issue that would be nice to fix.

@aadcg aadcg added the 4-series Related to releases whose major version is 4. label Apr 9, 2024
@shamazmazum
Copy link
Contributor Author

@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 input->queries or make-completion-query with :check-dns-p from which I removed that argument? Btw, they are not even exported. With exception of these two functions API is the same.

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 initialize-instance but leave :check-dns-p be. I thought it was better to remove it because it is not needed in buffer.lisp at all.

So what should I do?

@aadcg
Copy link
Member

aadcg commented Apr 9, 2024

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 input->queries or make-completion-query with :check-dns-p from which I removed that argument? Btw, they are not even exported. With exception of these two functions API is the same.

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.

So what should I do?

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:

a smaller fix which just fixes a navigation to URLs like "example.com/foo", I can just fix an erroneous conditional in initialize-instance but leave :check-dns-p be.

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?

@shamazmazum shamazmazum mentioned this pull request Apr 9, 2024
@shamazmazum
Copy link
Contributor Author

Yes, I've opened a new PR which fixes just the navigation.

@shamazmazum shamazmazum changed the title Fix set-url Remove :CHECK-DNS-P and DNS lookups (pending) Apr 10, 2024
@aadcg
Copy link
Member

aadcg commented Apr 17, 2024

Done, see #3385 (comment).

@aadcg aadcg closed this Apr 17, 2024
@aadcg aadcg removed the 4-series Related to releases whose major version is 4. label Apr 17, 2024
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.

None yet

2 participants