-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Included tag search capabilities #32045
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not really a fan of allowing end users to set SQL LIKE statements (i.e. they might not expect that entering
v1_%
shows all tags in Version 1X).However, what would be the alternative?
Parsing a glob instead and mapping it to the LIKE syntax?
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.
If we choose to merge this as it is, please add a docs page in https://gitea.com/gitea/docs describing that this search accepts the SQL LIKE syntax.
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.
The ideia behind is to macht all tags whose related commit message contains the criterion (i.e., if the user passes "2" as the criterion. It should matches v2, v1.2, v.1.32, and so on). I double-checked the generated SQL and xorm puts the wildcard on both sides of the literal (see evidence attached). This behavior is inline with how the commit search works.
About the documentation changes, gonna submitt a PR latter today :)
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.
It uses
builder.Like{"lower_tag_name", strings.ToLower(opts.NamePattern.Value())}
, which will generate a SQL likelower_tag_name = ?
, And the%
will be added automatically if it hasn't been added. If you search with '2%', then the '%' will not be added automatically.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.
@delvh do you think we still need to add a docs page for this particular change after the explanation ? I can't seem to find any existing page that is closely related to this change (the closest seem to be
Usage > Protected tags
but I feel is not quite right). Any ideas ?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.
My problem is simply that I don't like hiding functionality from the user in intransparent ways.
What we could do as a simple workaround is add a tooltip (
data-tooltip-content=…
attribute) to the search bar statingSearch for matching tags. Use '%' as a placeholder for any sequence of chars and '_' for a single unknown char
or something like that.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.
@delvh Cool, I'm gonna add the tooltip latter today :)
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.
@delvh What do you think ?
If you guys think it is ok, I'll push the changes :)
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.
Last thing: I'm not quite happy with using the query param
q
as tag name.I mean… it is sort of a query.
However, it might be confusing in the future if there are multiple search boxes why one is
q
while the other one isn't.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.
Hey @delvh, this one could be a little tricky :(
The thing is that the page is using the combo.tmpl template. Which, in turn uses the input,tmpl. The latter (source-code attached) assumes the name of the field to be
q
. I could change it to receive a custom name, but it feels that it is out of the scope of this particular PR.How do you guys want to proceed ? Do you want me to create another PR that allows a custom name for the combo component and them rebase the this one when the new one is merged ? Or do you want me to put it all together on a single PR ?