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

search: order non-main index entries after other results #11696

Merged
merged 8 commits into from
Mar 18, 2024

Conversation

bradking
Copy link
Contributor

Since #10819, index entries are returned as search results. When the query string exactly matches an indexed term, all index entries become search results with score 100. This places them above most other search results.

Reporting "main" index entries early in the search results makes sense, but non-main index entries are often just arbitrary cross-references to the indexed term. Collect them in a separate group that is always placed after other results. This avoids obscuring the main entries and other more-important matches such as document titles.

Fixes: #11578
Supersedes: #11695

Since commit 8ae8183 (Support searching for index entries (sphinx-doc#10819),
2022-09-20, v5.2.0~16), index entries are returned as search results.
When the query string exactly matches an indexed term, all index
entries become search results with score 100.  This places them above
most other search results.

Reporting "main" index entries early in the search results makes sense,
but non-main index entries are often just arbitrary cross-references to
the indexed term.  Collect them in a separate group that is always
placed after other results.  This avoids obscuring the main entries and
other more-important matches such as document titles.

Fixes: sphinx-doc#11578
sphinx/themes/basic/static/searchtools.js Outdated Show resolved Hide resolved
sphinx/themes/basic/static/searchtools.js Outdated Show resolved Hide resolved
sphinx/themes/basic/static/searchtools.js Outdated Show resolved Hide resolved
sphinx/themes/basic/static/searchtools.js Show resolved Hide resolved
@bradking
Copy link
Contributor Author

bradking commented Oct 3, 2023

I added a merge commit to resolve conflicts with master. Please let me know if I should rebase instead.

@picnixz
Copy link
Member

picnixz commented Oct 4, 2023

I think it's fine since we squash the commits at the end.

@bradking bradking requested a review from picnixz October 6, 2023 15:08
@picnixz
Copy link
Member

picnixz commented Oct 7, 2023

I'll review it by the end of this weekend. Seems fine for me though (but @AA-Turner should review as well). By the way, could the OP confirm that this PR will not break existing project? Like, is it ensured that nonmain index entries are really irrelevant compared to main ones in general?

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

For me it's fine. But could I know its real effect on existing projects such as the Python docs (compared to the original issue where we introduced this search). Also, did it actually fix the CMake documentation?

sphinx/themes/basic/static/searchtools.js Outdated Show resolved Hide resolved
@bradking
Copy link
Contributor Author

this PR will not break existing project? Like, is it ensured that nonmain index entries are really irrelevant compared to main ones in general?

AFAICT the original motivation was only for main entries. #6692 requests support for the "index directive". Review of #10819 did not raise the distinction.

did it actually fix the CMake documentation?

Yes. That's what's motivating this PR.

@bradking bradking requested a review from picnixz October 24, 2023 13:39
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Ah sorry! as I said on another post, I am quite unable to review anything. However, if everything works well for both CMake and Python, then I have nothing more to

By the way, I don't have any write access and we usually wait for Adam's reviews before merging (sometimes, I miss obvious issues) but since they're also busy, I don't know when this will be reviewed.

@bradking
Copy link
Contributor Author

@picnixz @AA-Turner I've updated this PR to resolve conflicts with master. Please review again.

@picnixz
Copy link
Member

picnixz commented Feb 1, 2024

Errr.. for me it's fine? I mean, I don't think there has been any difference with what I've already reviewed and the fact that no changes should occur with CMake and Python is a good indication. Since I'm only passing by, I don't want to merge now (if there's an issue afterwards, I won't be able to react fast enough) so I'll let @AA-Turner have a look as well.

@jayaddison jayaddison added html search javascript Pull requests that update Javascript code labels Mar 3, 2024
@jayaddison jayaddison added the awaiting:review PR waiting for a review by a maintainer. label Mar 14, 2024
@picnixz
Copy link
Member

picnixz commented Mar 17, 2024

I think it's better if we don't wait too long for that one. I'll endorse the merge (just resolve the conflicts and we're good to go).

@picnixz picnixz added priority:high and removed awaiting:review PR waiting for a review by a maintainer. labels Mar 17, 2024
@picnixz picnixz merged commit a9383a0 into sphinx-doc:master Mar 18, 2024
23 checks passed
@picnixz
Copy link
Member

picnixz commented Mar 18, 2024

Thank you!

@bradking bradking deleted the search-result-grouping branch March 18, 2024 16:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
html search javascript Pull requests that update Javascript code priority:high type:bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

searchindex: non-main index entries scored too high
3 participants