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

Duplicate msgid with singular and plural form #366

Closed
saveman71 opened this issue Jun 9, 2023 · 5 comments · Fixed by #379
Closed

Duplicate msgid with singular and plural form #366

saveman71 opened this issue Jun 9, 2023 · 5 comments · Fixed by #379
Assignees
Labels

Comments

@saveman71
Copy link

saveman71 commented Jun 9, 2023

Not sure if it's a bug / how other implementations tackle that, but msgfmt will fail for duplicate msgids:

❯ msgfmt -c priv/gettext/default.pot
priv/gettext/default.pot:10: duplicate message definition...
priv/gettext/default.pot:5: ...this is the location of the first definition
msgfmt: priv/gettext/default.pot: warning: PO file header missing or invalid
                                  warning: charset conversion will not work
msgfmt: found 1 fatal error

Which is expected for cases like:

#, elixir-autogen, elixir-format
msgid "Singular."
msgstr ""

#, elixir-autogen, elixir-format
msgid "Singular."
msgstr ""

However, it also fails for cases like:

#, elixir-autogen, elixir-format
msgid "Singular."
msgid_plural "Plural."
msgstr[0] ""
msgstr[1] ""

#, elixir-autogen, elixir-format
msgid "Singular."
msgstr ""

Generated by this code:

defmodule GettextMwe do
  import GettextMwe.Gettext

  @moduledoc false

  def singular do
    gettext("Singular.")
  end

  def plural(count) do
    ngettext("Singular.", "Plural.", count)
  end
end

I believe things should work fine if the singular form was dropped from the po/pot file.

Here's the repro repo: https://github.com/saveman71/elixir-gettext-duplicate-keys

Is this a bug, or an accepted behaviour and msgfmt is just being too noisy/plain wrong here?

The issue I can see is that usually both translations aren't always used in the same context, so "merging" them could be a breaking change.

msgfmt allowed me to remove actual duplicates, but now its output is only this case, so I can't move forward adding it as a new CI step.

@whatyouhide
Copy link
Contributor

We can't merge the translations yeah. I mean, the Elixir Gettext behavior seems fine to me because the singular and plural translations are treated completely differently, so they can coexist with the same msgid.

Is msgfmt the only thing that fails on this in the GNU Gettext toolset? Also, do you know what GNU Gettext does when extracting in situations when this would happen? For example, I don't know how PHP's ngettext or other implementations deal with this.

@maennchen
Copy link
Member

@whatyouhide PHP reads the msgid of a plural translation as well:

https://github.com/php/php-src/blob/9e15910889cfa1f14169ed9babf3bf54426cca1c/ext/gettext/tests/gettext_dgettext.phpt#L23

var_dump(gettext('item'));
[...]
string(7) "Produkt"

https://github.com/php/php-src/blob/afffcaeb4d994be50c83ac0204b4d4be8af560d9/ext/gettext/tests/locale/en_US.UTF-8/LC_MESSAGES/dngettextTest.po#L1

msgid "item"
msgid_plural "items"
msgstr[0] "Produkt"
msgstr[1] "Produkte"

@maennchen
Copy link
Member

maennchen commented Nov 3, 2023

Judging by this paragraph, I think that is the expected way to do things:

https://www.gnu.org/software/gettext/manual/gettext.html#Invoking-the-msguniq-Program

The msguniq program unifies duplicate translations in a translation catalog. It finds duplicate translations of the same message ID. Such duplicates are invalid input for other programs like msgfmt, msgmerge or msgcat. By default, duplicates are merged together. [...]

So I guess we should merge singular and plural into one plural message and use the msgid of plural messages for singular calls.

@whatyouhide
Copy link
Contributor

Yeah I think that's a good idea. I will be out all of next week so it will be a while before I'll be able to do anything about this 🙃

maennchen added a commit that referenced this issue Nov 6, 2023

Verified

This commit was signed with the committer’s verified signature.
maennchen Jonatan Männchen
@maennchen maennchen self-assigned this Nov 6, 2023
@maennchen maennchen linked a pull request Nov 6, 2023 that will close this issue
@saveman71
Copy link
Author

Sorry I was out of office and not great at using GitHub's notification center 😅 Thanks for looking into this!

maennchen added a commit that referenced this issue Nov 9, 2023

Verified

This commit was signed with the committer’s verified signature.
maennchen Jonatan Männchen
maennchen added a commit that referenced this issue Nov 15, 2023

Verified

This commit was signed with the committer’s verified signature.
maennchen Jonatan Männchen
maennchen added a commit that referenced this issue Nov 29, 2023

Verified

This commit was signed with the committer’s verified signature.
maennchen Jonatan Männchen
maennchen added a commit that referenced this issue Dec 4, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants