Skip to content

Fix Docs Search in Sphinx 6.0+ #3498

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
Merged
Bibo-Joshi merged 9 commits into from
Feb 3, 2023
Merged

Fix Docs Search in Sphinx 6.0+ #3498

Bibo-Joshi merged 9 commits into from
Feb 3, 2023

Conversation

@harshil21
Copy link
Member

@harshil21 harshil21 commented Jan 10, 2023 •
edited
Loading

Fixes doc search in sphinx 6.0+. (This is not fixed at readthedocs-sphinx-search yet Was fixed and merged into my fork)

Build at: https://docs.python-telegram-bot.org/en/doc-improve-search

Changelog at: https://github.com/harshil21/furo-sphinx-search/commits/main (see commit msgs)

Todo now that I started this again:

  • Sort search results such that highlighted results on the left are prioritized over ones only on the right
  • Experimental: Prefix search if only one word has been typed, so we get better results.

@harshil21 harshil21 added ⚙️ documentation affected functionality: documentation ⚙️ dependencies affected functionality: dependencies labels Jan 10, 2023
@harshil21 harshil21 added this to the v20.1 milestone Jan 10, 2023
@harshil21 harshil21 added the 📋 do-not-merge-yet work status: do-not-merge-yet label Jan 10, 2023
@harshil21
Copy link
Member Author

harshil21 commented Jan 11, 2023

Auto modification of search query seems to be futile.. and since API v3, most search results seem to be sorted anyway, so that is not worth the effort.

PR can be merged/reviewed now

@harshil21 harshil21 removed the 📋 do-not-merge-yet work status: do-not-merge-yet label Jan 11, 2023
@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 11, 2023

Thanks for the PR!

Auto modification of search query seems to be futile.

Can you remind me what "auto modification of search query" means? 😬

This is not fixed at readthedocs-sphinx-search yet

IMO it's worth a try to PR against upstream with these changes first. If they adopt the proposed changes, you can merge into your branch. This reduces the risk of merge conflicts. However, I don't know if your branch has already diverged too much from upstream, i.e. if your changes are not applicable to upstream anymore, that's ofc a different story.

@harshil21
Copy link
Member Author

harshil21 commented Jan 12, 2023

Can you remind me what "auto modification of search query" means? 😬

I mean this. I hoped that doing something like send_mes* (the * is added to the query automatically) give send_message as a search result, but it does not and instead it makes it worse in some cases, e.g. send_message* does not give you Bot.send_message as a result (I would expect it to). Their backend for search is very poor imo.

IMO it's worth a try to PR against upstream with these changes first.

I did open an issue yesterday (readthedocs/readthedocs-sphinx-search#126), no response yet

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 12, 2023

Their backend for search is very poor imo.

They = Sphinx?

I saw the issue, but I didn't see a PR yet

@harshil21 harshil21 added the 📋 pending-reply work status: pending-reply label Feb 1, 2023
@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Feb 1, 2023

readthedocs/readthedocs-sphinx-search#126 has been resolved by now. Can you update your branch accordingly?

@harshil21
Copy link
Member Author

harshil21 commented Feb 1, 2023

readthedocs/readthedocs-sphinx-search#126 has been resolved by now. Can you update your branch accordingly?

yeah will do tomorrow or day after

@harshil21 harshil21 added 📋 pending-merge work status: pending-merge and removed 📋 pending-reply work status: pending-reply labels Feb 3, 2023
@Bibo-Joshi Bibo-Joshi merged commit b5672f0 into doc-fixes Feb 3, 2023
@Bibo-Joshi Bibo-Joshi deleted the doc-improve-search branch February 3, 2023 17:30
@Bibo-Joshi Bibo-Joshi mentioned this pull request Feb 3, 2023
Merged
9 tasks
@harshil21 harshil21 removed the 📋 pending-merge work status: pending-merge label Feb 4, 2023
Bibo-Joshi added a commit that referenced this pull request Feb 5, 2023
@Bibo-Joshi @harshil21
@shivamsn97 @clot27 @lemontree210 @CrsiX @Poolitzer
Documentation Improvements (#3464, #3483, #3484, #3497, #3512, #3501, #…
9953216 
…3515, #3523,  #3498, #3529)

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Shivam Saini <51438830+shivamsn97@users.noreply.github.com>
Co-authored-by: Aditya Yadav <adityayadav11082@gmail.com>
Co-authored-by: Dmitry Kolomatskiy <58207913+lemontree210@users.noreply.github.com>
Co-authored-by: Crsi <47722349+CrsiX@users.noreply.github.com>
Co-authored-by: poolitzer <github@poolitzer.eu>
Co-authored-by: Aditya <clot27@apx_managed.vanilla>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

⚙️ dependencies affected functionality: dependencies ⚙️ documentation affected functionality: documentation

Projects

None yet

Milestone

v20.1

Development

Successfully merging this pull request may close these issues.

3 participants

@harshil21 @Bibo-Joshi