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.

Sign up for GitHub

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

Jump to bottom

Allow searching by Issue Index #19646

Closed
wants to merge 7 commits into from
Closed

Allow searching by Issue Index #19646

wants to merge 7 commits into from
+21 −2

Conversation

6543
Copy link
Member

@6543 6543 commented May 7, 2022

commits taken from #17929

lukas and others added 6 commits December 10, 2021 08:25
Allow searching by issue ID

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
95f5017 
Add issue ID to found issues if it is a string
@wxiaoguang
Merge branch 'main' into issueSearch

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
29cb723
@wxiaoguang
Merge branch 'main' into issueSearch

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
098a4d6
@wxiaoguang
Merge branch 'main' into issueSearch

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
9f3d0cc
@zeripath
Merge remote-tracking branch 'origin/main' into issueSearch

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
a16b958
@6543
Merge branch 'master' into issueSearch

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
fbed27d
@6543 6543 added the type/enhancement An improvement of existing functionality label May 7, 2022
@6543 6543 added this to the 1.17.0 milestone May 7, 2022
@6543 6543 mentioned this pull request May 7, 2022
Closed
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 7, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 7, 2022
@6543
Copy link
Member Author

6543 commented May 7, 2022

mysql integration test fails - could be an elastic search related issue :/

@6543
Copy link
Member Author

6543 commented May 7, 2022

problem: test dont show errors!!! (https://drone.gitea.io/go-gitea/gitea/54666/2/14)

@6543 6543 added the pr/wip This PR is not ready for review label May 7, 2022
@zeripath
Merge branch 'main' into issueSearch

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
07e2c0b
@lunny
Copy link
Member

lunny commented May 7, 2022 •
edited
Loading

Isn't we can get the issues via index from databases? Why we need this PR?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 7, 2022 •
edited
Loading

Isn't we can get of issues via index from databases? Why we need this PR?

At that time (I reviewed the original PR), I think that changing the search logic is more heavy than adding a search field.

For example, if you input "12345", it might be an issue index, it might be an keyword to be searched, so putting it into the search engine is quite simple.

Indeed, this PR is a nice to have, not a must. We can teach users to input the issue index in browser's address bar directly too, it depends how we think. So, merging it or not, either is fine for me.

I just took the original PR as an example, even if we think a PR is not suitable, we had better tell the contributors at the first time, it could make contributors feel better.

lunny
lunny reviewed May 7, 2022
View reviewed changes
modules/indexer/issues/bleve.go
@@ -244,6 +247,7 @@ func (b *BleveIndexer) Search(ctx context.Context, keyword string, repoIDs []int
for _, repoID := range repoIDs {
repoQueriesP = append(repoQueriesP, numericEqualityQuery(repoID, "RepoID"))
}
index, _ := strconv.ParseInt(keyword, 10, 64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to check if index > 0 to decide to if we need search the field.

@lunny
Copy link
Member

lunny commented May 7, 2022

When you input #123 or a/b#123 in the search box, we could directly go to issue 123. But when you input 123, should we search the issue with index 123? I think it's annoying and unnecessary.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 7, 2022 •
edited
Loading

When you input #123 or a/b#123 in the search box, we could directly go to issue 123. But when you input 123, should we search the issue with index 123? I think it's annoying and unnecessary.

Yep, that's also a solution. The PR author didn't accept it before. Refer to the original PR, it has been discussed.

image

@wxiaoguang
Copy link
Contributor

Since there is no new requirement for this feature, and there is still divergence on it.

I would propose to convert it to a draft, leave it as it is, until there is some new thoughts or new requirements about it. (maybe by some RFC in the future).

I don't think we should merge it just because someone has approved it at the moment.

@wxiaoguang wxiaoguang modified the milestones: 1.17.0, 1.x.x May 25, 2022
@wxiaoguang wxiaoguang marked this pull request as draft May 25, 2022 16:11
@wxiaoguang wxiaoguang removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label May 31, 2022
@tyroneyeh tyroneyeh mentioned this pull request Sep 23, 2022
Closed
5 tasks
@lunny lunny removed this from the 1.x.x milestone Jan 18, 2023
@wxiaoguang wxiaoguang mentioned this pull request May 2, 2023
Merged
@wxiaoguang
Copy link
Contributor

Will be replaced by #24479

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label May 2, 2023
@wxiaoguang wxiaoguang closed this May 8, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny

@zeripath zeripath

@wxiaoguang wxiaoguang

@delvh delvh

Assignees
No one assigned
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail pr/wip This PR is not ready for review type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@6543 @lunny @wxiaoguang @zeripath @delvh @GiteaBot