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

Reorder parameters in tr_n and related methods #66292

Closed
wants to merge 1 commit into from

Conversation

dalexeev
Copy link
Member

Make n parameter first and plural_message optional in the following methods:

  • Object.tr_n();
  • Translation._get_plural_message() (only order);
  • Translation.get_plural_message();
  • TranslationServer.translate_plural();
  • TranslationServer::tool_translate_plural();
  • TranslationServer::doc_translate_plural();
  • TTRN(), RTRN(), DTRN().

When using translations based on identifiers, the plural_message parameter is not needed, so it makes sense to make the n parameter first and plural_message optional. In my opinion, this is logical and convenient, since the n parameter is "the most required" parameter in the tr_n function.

These changes are in PR #41519, but it's very likely that #41519 might not make it into the 4.0 release. This PR will avoid breaking compatibility if #41519 is implemented later. Also, it will be useful if someone decides to implement a custom translation format based on identifiers (it's possible, i'm going to implement my format instead of CSV).

@dalexeev dalexeev requested review from a team as code owners September 23, 2022 10:03
@@ -109,10 +109,10 @@ class ExtractType(enum.IntEnum):
re.compile(r'TTR\("(?P<message>([^"\\]|\\.)*)"(, "(?P<context>([^"\\]|\\.)*)")?\)'): ExtractType.TEXT,
re.compile(r'TTRC\("(?P<message>([^"\\]|\\.)*)"\)'): ExtractType.TEXT,
re.compile(
r'TTRN\("(?P<message>([^"\\]|\\.)*)", "(?P<plural_message>([^"\\]|\\.)*)",[^,)]+?(, "(?P<context>([^"\\]|\\.)*)")?\)'
r'TTRN\([^,]+?, "(?P<message>([^"\\]|\\.)*)", "(?P<plural_message>([^"\\]|\\.)*)"(, "(?P<context>([^"\\]|\\.)*)")?\)'
Copy link
Member Author

@dalexeev dalexeev Sep 23, 2022

Choose a reason for hiding this comment

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

For TTRN(), RTRN(), DTRN() I think plural_message should be left required? (Otherwise, this regular expression also needs to be corrected.)

@dalexeev
Copy link
Member Author

This PR does not break compatibility with 3.x, but breaks with 4.0 beta 3.

@timothyqiu
Copy link
Member

I can see the original intention of reordering parameters: in order to make plural_message optional, it has to be after non-optional arguments. So moving n to the first becomes a requirement.

But what's the reason for making plural_message optional? #41519 says it's unused when using identifier as message id. TBH, I think using identifier as message id is another layer of abstraction on top of tr functions and po/csv files. This usage should not affect how the underlying function works.

tr_n("%d day ago", "%d days ago", days)  # As regular text.

tr_n("DAYS_AGO", "DAYS_AGO", days)  # Repeat the identifier: this is also confusing when extracted to POT.

# Make a wrapper to avoid repeating yourself.
func tr_identifier(identifier: String, n: int) -> StringName:
    return tr_n(identifier, identifier, n)

tr_n() uses the same order as the pseudo standard ngettext() function. I don't think it should be reordered.

@dalexeev
Copy link
Member Author

Make a wrapper to avoid repeating yourself.
func tr_identifier(identifier: String, n: int) -> StringName:
    return tr_n(identifier, identifier, n)

This is less convenient because GDScript does not support user-defined global functions. It will be at least Util.my_tr_n.

tr_n() uses the same order as the pseudo standard ngettext() function.

Compliance with standards is good, but is it really that important in this case? Should we follow the standard if it's less convenient?

I think those who are used to the order of arguments in ngettext will quickly get used to the different order in tr_n. At most they will be a little confused the first time, but the function signature hint and type mismatch will help fix the error.

At the same time, those who use CSV and perhaps don't know about ngettext at all will be inconvenienced by the extra argument every time.

@timothyqiu
Copy link
Member

Yeah, compliance with standards is good to have but not a requirement. We already merged the context feature from p*gettext() into tr() and tr_n().

My main argument is that tr functions are designed for plain texts instead of identifiers. I think using identifiers in tr() works by coincidence, it does not mean tr_n() should work for identifiers too.

Threre will still be problems after reordering the parameters. For example, if we were to implement context support for CSV files, users still have to write something like tr_n(days, "DAYS_AGO", "", "calendar").

I think it's better to use a wrapper function for translating identifiers.

@dalexeev
Copy link
Member Author

dalexeev commented Oct 22, 2022

My main argument is that tr functions are designed for plain texts instead of identifiers.

Yes, it looks more readable with original strings than with IDs. But in my experience, a lot of people use tr function with CSV and IDs instead of .po files, especially those who don't speak English. And if the original language has more than one plural form, IDs are preferred, since you simply cannot pass all forms to tr_n.

For example, if we were to implement context support for CSV files, users still have to write something like tr_n(days, "DAYS_AGO", "", "calendar").

Context is needed to disambiguate when the same phrase in the source language is translated into several different phrases depending on the context. However, IDs must initially be unique, the context is included in the ID name. Therefore, this scenario seems impossible in practice. (It should be DAYS_AGO_CALENDAR or something similar.)

I think it's better to use a wrapper function for translating identifiers.

The wrapper function is one of the possible solutions to the problem, but it's less convenient to use at the moment, due to the limitations of GDScript. Also, if we can make the standard function more convenient, that looks like a better solution, I think.

@dalexeev dalexeev force-pushed the tr-n-args branch 2 times, most recently from 38d041c to 86979c2 Compare November 26, 2022 14:28
@dalexeev dalexeev requested a review from a team as a code owner November 26, 2022 14:28
@dalexeev
Copy link
Member Author

This PR breaks compatibility with 4.0 beta, add the label please.

Make `n` parameter first and `plural_message` optional
in the following methods:

* `Object.tr_n()`;
* `Translation._get_plural_message()` (only order);
* `Translation.get_plural_message()`;
* `TranslationServer.translate_plural()`;
* `TranslationServer::tool_translate_plural()`;
* `TranslationServer::doc_translate_plural()`;
* `TTRN()`, `RTRN()`, `DTRN()`.
@dalexeev
Copy link
Member Author

dalexeev commented Feb 9, 2023

Waiting for Godot 5. :|

@dalexeev dalexeev closed this Feb 9, 2023
@aaronfranke aaronfranke removed this from the 4.x milestone Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants