-
Notifications
You must be signed in to change notification settings - Fork 16
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
Prevent search for invalid queries #34
base: main
Are you sure you want to change the base?
Conversation
@@ -5,16 +5,27 @@ defmodule DiffWeb.SearchLiveView do | |||
DiffWeb.SearchView.render("search.html", assigns) | |||
end | |||
|
|||
@valid_query ~r{^[a-zA-Z0-9_]+$} |
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.
We should use this regex: https://github.com/hexpm/hexpm/blob/master/lib/hexpm/repository/package.ex#L67.
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.
Should we though? To me a query that starts with a number (say, 3339) or a capital letter seems fairly legitimate. 🤔
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 feel that validating the queries is an improvement here. Looking up package names is not an expensive operation, it's easier to just let users search for what they want. And suggestions will still work, even if the query includes characters that are not allowed in Hex package names, eg typing in "pho!nix" will suggest "phoenix".
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.
And suggestions will still work, even if the query includes characters that are not allowed in Hex package names, eg typing in "pho!nix" will suggest "phoenix".
That's a good point. The reason why this idea came to me in the first place was that it's not uncommon for people with non-latin keyboards (including myself) to forget to switch, thus, say phoenix
would become зрщутшч
, ending up in multiple completely wasted search attempts. Perhaps adding some kind of filter would be a better option. Or maybe something even more exciting, such as a mapping mechanism that would transform the aforementioned зрщутшч
back into phoenix
? Although the latter would seem fairly unrealistic given the amount of languages in the world.
There seems to be no good reason to trigger a search if the query is invalid.
Apparently fixes this issue.