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

ngettext() shouldn't be used to pluralize formally in some cases #6796

Open
MichaelChirico opened this issue Feb 4, 2025 · 2 comments
Open
Labels
translation issues/PRs related to message translation projects
Milestone

Comments

@MichaelChirico
Copy link
Member

Thank you very much for collecting these!

One more typo (extra ) at the end):

Rprintf(_("RHS for item %d has been duplicated because MAYBE_REFERENCED==%d MAYBE_SHARED==%d, but then is being plonked. length(values)==%d; length(cols)==%d)\n"),

I would also like to replace the use of ngettext for strings that don't format the count as a number because that prevents proper translation with plurals in some languages, but that can be a separate issue. For your example, that would be

stop_msg = if (length(tt) == 1) {
  gettext("The item in the 'by' or 'keyby' list is length %s. Each must be length %d; the same length as there are rows in x (after subsetting if i is provided).")
} else {
  gettext("The items in the 'by' or 'keyby' list have lengths %s. Each must be length %d; the same length as there are rows in x (after subsetting if i is provided).")
}
stopf(stop_msg, brackify(tt),  xnrow, domain = NA)

Originally posted by @aitap in #6787

See this discussion in #6786. There's a proposed patch there but it needs to be paired with some improvements to {potools}, I think. Tabling for now until after release.

@MichaelChirico MichaelChirico added the translation issues/PRs related to message translation projects label Feb 4, 2025
@MichaelChirico MichaelChirico added this to the 1.18.0 milestone Feb 4, 2025
@venom1204
Copy link
Contributor

venom1204 commented Feb 5, 2025

Hi

Can I proceed with making the changes for Issue #6796 now, or would you prefer to revisit this after the release?
I’d like to work on replacing ngettext() with gettext() where necessary to ensure correct pluralization. Specifically, I will:

  1. Identify and change occurrences of ngettext() where the count is not explicitly formatted as a number (e.g., in error
    messages).
  2. Replace these with gettext() and manually handle pluralization.

Files I will modify:
R/utils.R ,R/fmelt.R ,R/data.table.R
These files use ngettext() incorrectly and need manual pluralization adjustments.

Files I will NOT modify:
R/bmerge.R ,R/setops.R ,R/fread.R ,R/print.data.table.R ,R/test.data.table.R ,R/data.table.R

These files already use ngettext() correctly, as they properly format the count using %d.
Let me know your preference. Thanks!

@MichaelChirico
Copy link
Member Author

Thanks @venom1204. This will definitely happen after release. Actually, the most important work for this issue needs to happen not in {data.table}, but in {potools}:

MichaelChirico/potools#317 (comment)

The linked PR here on {data.table} has a patch that will do the bulk of the work on our side; what remains is to ensure that this patch works well with the related tooling (i.e., {potools})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translation issues/PRs related to message translation projects
Projects
None yet
Development

No branches or pull requests

2 participants