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

Searchindex fix when objects have the same name #9649

merged 4 commits into from Sep 26, 2021


Copy link

@jakobandersen jakobandersen commented Sep 18, 2021

Feature or Bugfix

  • Bugfix


Store all objects with the same name from different domains in searchindex.js.


Objects in searchindex.js are stored as a dictionary indexed by the object name. However, it is perfectly reasonable to have, e.g., both a Python and C function with the same name. This PR changes the storage to an array.

To test out:

  1. Clone and build to HTML.
  2. Search for "f". This should list at least entries for fpy, fc, and fcpp.
  3. Search for g. This only lists the Python function, even though there is a C and C++ function of the same name.

Repeat the same steps with this PR should in step 3 yield all three g objects.


Started from #9574 (comment)..

Copy link

@tk0miya tk0miya commented Sep 18, 2021

LGTM. I'm not sure why the names are unique. I guess there is no reason to keep keywords unique. So +1 for this change.

BTW, we need to discuss about the side effect of this change. After this change, the structure of searchindex.js will be changed. It would be a breaking change if some 3rd party tool directly reads. AFAIK, there are no such tools. So I'm okay to merge this into the 4.x branch. But we need to announce it via CHANGES.

Copy link
Contributor Author

@jakobandersen jakobandersen commented Sep 25, 2021

@tk0miya, I have added a note to CHANGES to warn about it. While it is indeed a breaking change I think the chance of someone using searchindex.js is somewhat low. The code calls a method on a specific object, passing the index as an object, so if someone is using it they anyway need to mock the interface assumed.
Let me know if you are fine with the PR with this CHANGES entry (or feel free to merge it your self).

Copy link

@tk0miya tk0miya left a comment

LGTM. Merging!

@tk0miya tk0miya merged commit 72e5090 into sphinx-doc:4.x Sep 26, 2021
16 checks passed
@jakobandersen jakobandersen deleted the searchindex_fix branch Oct 1, 2021
Copy link
Contributor Author

@jakobandersen jakobandersen commented Oct 1, 2021


@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants