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

Fix singular and plural search results text #12639

Merged
merged 10 commits into from
Aug 11, 2024

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Jul 21, 2024

Feature or Bugfix

  • Bugfix

Purpose

  • Show accurate search results text.

Detail

Search for something and, if there are results, it will say something like "Search finished, found 1 page(s) matching the search query." or "Search finished, found 23 page(s) matching the search query."

Computers are bad at some things, but one thing they're very good at is counting :)

Let's show "1 page" for a single result, and "${resultCount} pages" for multiple.

Before

Shows "page(s)" for both single and multiple results:

image

https://docs.python.org/3/search.html?q=hotspot

image

https://docs.python.org/3/search.html?q=lru_cache

After

Shows "page" for a single result and "pages" for multiple results:

image image

Localisation

Do we need to do something to update the translation files?

@jayaddison
Copy link
Contributor

Note that there's an ngettext interface available in the JS code, intended to allow count-aware internationalization of messaging.

"plural_expr": "(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2)"

Could we use that to achieve a more internationalizable fix?

@hugovk
Copy link
Contributor Author

hugovk commented Jul 21, 2024

That sounds better, do you know how to use it?

@jayaddison
Copy link
Contributor

That sounds better, do you know how to use it?

I don't yet, no, but I'd be willing to learn in support of reviewing and/or suggesting an adjustment of the PR that uses it. That could take me a few days.

@jayaddison
Copy link
Contributor

Ah, and I remembered that I should probably spend some time refreshing my knowledge of other features to support an upcoming 8.0.0 release - so I'd update my timeline here to a couple of weeks or so to provide support (possibly pessimistic, but I'd prefer that than overcommitting).

@hugovk
Copy link
Contributor Author

hugovk commented Jul 22, 2024

Yeah, no rush at all for this one :)


Right, so _ is an alias for Documentation.gettext, and there's an ngettext for dealing with singular and plural forms:

https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html

The next step is updating the translation files with these strings.

@hugovk
Copy link
Contributor Author

hugovk commented Jul 22, 2024

There are some instructions at https://www.sphinx-doc.org/en/master/internals/contributing.html#translations

I ran python babel_runner.py extract and python babel_runner.py update but not python babel_runner.py compile.

@jayaddison
Copy link
Contributor

That's great - I wasn't certain whether babel_runner.py would detect the ngettext(...) call - I thought we might have to adjust the callsite to use __(...) instead, based on the defined keywords. But indeed ngettext is in the babel defaults (and must pattern-match OK regardless of reference from a Documentation class).

@AA-Turner AA-Turner added this to the 8.x milestone Jul 22, 2024
@jayaddison
Copy link
Contributor

This will also need a CHANGES.rst entry - those usually reference a separate bugreport number, but it is also valid to self-reference the PR number.

Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

This is a nice improvement for the quality of search result presentation and localization.

@jayaddison
Copy link
Contributor

@AA-Turner aside from the merge conflict resolution, is there anything else to resolve before this could be considered for merge?

@AA-Turner AA-Turner merged commit 9171f53 into sphinx-doc:master Aug 11, 2024
21 checks passed
@jayaddison
Copy link
Contributor

Thank you @hugovk @AA-Turner!

@hugovk hugovk deleted the singular-plural-search-results branch August 11, 2024 19:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2024
@AA-Turner AA-Turner removed this from the 8.x milestone Oct 6, 2024
@AA-Turner AA-Turner added this to the 8.1.0 milestone Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants