-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
HTML Search: fixup: include partially-matched document titles in search results. #12041
HTML Search: fixup: include partially-matched document titles in search results. #12041
Conversation
…t-match results Co-authored-by: William Lachance <[email protected]>
This reverts commit 23da660.
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.
Looks right to me
Thanks @wlach! |
…on entirely when we have already found exact-matches
…040/include-partial-title-matches-in-search-results Conflicts: sphinx/themes/basic/static/searchtools.js
…in-search-results
…in-search-results
…in-search-results
Ok, I'm just picking a random PR on search to ask something: @jayaddison @wlach Could you create an issue where you gather all the search-related PRs and the ones that should be merged (or alternatives)? like, which one should I focus first etc, whether one PR should be merged before another or not etc? They are a bit spread everywhere and it's awfully hard for me to pick the correct one. |
Hi @picnixz - the remaining pull requests can be considered independently of each other. I've repurposed the Logically I think #11578 would be the first one to merge, followed by #12041. Two of the pull requests are competing/alternate solutions to the same problem; we think we've decided on our preferred pull request -- but even so, if possible, I'd like if someone with long-term experience of the search code to catch any mistakes with that (because my sense is that it could be trickier than some other changes to rollback/revert problems). |
I can create another label for that (awaiting:review) |
@picnixz ah - I noticed that you were awaiting further review from @AA-Turner on #11578. In that case I think #12041 is the one to start with (hopefully you notice this in time).
Thanks! |
So 12041 is actually this one. Just wondering, but the partially matched documents are put after those with exact match, right? |
Correct, yep. Exact-match titles are given a score of That does make me think that ideally one of the duplicate-result fixes might be better to merge first.. only for people who are installing from our |
Ok, then this PR is fine (just resolve the conflicts and I'll merge it) |
…in-search-results
…in-search-results
Nearly-ready, but I think I'll block/wait on this until the JS search tests are refactored. |
…in-search-results
@jayaddison I know you said that you would be offline but since nothing changed since last time (except conflicts and CHANGES), I assume that everything is fine, right? if so, I'll merge it this afternoon. |
@picnixz yep; this is ready and can be merged without other dependencies, so I felt it could be good to mark as ready for review while I'm taking a rest. Thank you. |
Thank you both! |
Feature or Bugfix
Purpose
sphinx_utils module
should be found when searching forsphinx
, but currently is not).Detail
word
from the user query (sphinx
, to continue the example) instead of the partially-matching term (sphinx_utils
).Relates