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

Implemented TranslationServer::format_integer #28710

Closed
wants to merge 1 commit into from
Closed

Implemented TranslationServer::format_integer #28710

wants to merge 1 commit into from

Conversation

MuffinManKen
Copy link
Contributor

At the moment this only supports a single locale ("en"), but I wanted to get some feedback on the code first to see if I'm headed in a Godot-compatible direction.

Addresses #28660

…dn't find info on is left with a commented out placeholder and will default to the en_US values.
@MuffinManKen
Copy link
Contributor Author

If everything looks okay, I think this can be merged. As mentioned in the commit log, not all locales have data but:

  1. There's a commented line for each locale missing data so it's easy to track/update
  2. These locales will end up getting the en_US values, which should be correct more often than not.

@MuffinManKen MuffinManKen changed the title First cut at implementing TranslationServer::format_integer. Requesting feedback Implemented TranslationServer::format_integer May 21, 2019
@akien-mga
Copy link
Member

The feature is interesting, but we don't use the C++ STL in Godot's codebase, so you would have to do it without sstream and std::locale.

@akien-mga akien-mga removed request for a team, reduz and neikeq May 28, 2019 08:44
@Calinou
Copy link
Member

Calinou commented Sep 5, 2019

@MuffinManKen Bump 🙂

In a future PR, we could also add currency sign information to each locale; this way, currencies can be formatted with the correct sign position. In some languages such as English, the currency symbol is placed before the number, but in many other languages, it goes after the number. See vue-currency-filter for examples of other properties that could be added (such as a decimals separator when TranslationServer::format_float is implemented).

Formatting currencies will require the user to specify the currency sign, which could be an optional second parameter to the TranslationServer::format_float function (and perhaps TranslationServer::format_integer). Still, this is low-priority as the editor doesn't display currencies anywhere.

@MuffinManKen
Copy link
Contributor Author

@Calinou Agreed. There's a lot of room for improvement here. I've been swamped with other work, but definitely plan to get back to this.

@akien-mga
Copy link
Member

Before doing too much work to improve the PR, we need to reach a decision on whether this is a wanted feature and the right approach to do it.

The feature makes sense to me, but reimplementing a table of all locale-specific number formatting sounds pretty tedious and overkill. Beyond possible separators in large numbers, there's also different decimal separators depending on the locale. Bad handling of decimal separator has actually plagued Unity games for years on Linux due to them picking up the system decimal separator (e.g. , on fr_FR.UTF-8) and trying to parse JSON with it... So in a sense it's likely safer not to provide too much functionality out of the box in this regard.

If we really want such feature, it likely makes more sense to defer the handling of all locales' particularities to a dedicated library, but I'm not sure we would want to pull in a full blown i18n library for that. Maybe we can defer to OS calls if they provide this information for the user's current locale.

@MuffinManKen
Copy link
Contributor Author

I don't think that it's often needed to have the functionality of a full i18n library. Parsing data based on the current locale seems like a really odd thing to do, but I suppose that's an artifact of trying to do localization "automagically".

The goal of this PR was to make it easier for developers to explicitly format numbers in a locale-aware way. If this and float/money formatting were provided I think that would cover 95% of what most developers would need.

I don't like the idea of using the OS's current locale. I see 2 problems with it right away:

  1. Testing. If I, or my testers, want to make sure my game doesn't blow up when set to Russian, I don't want to change the OS's locale to do that.

  2. It's not uncommon for players to set their game locale to something different than what their OS is set to.

  3. It would be inconsistent. If my game doesn't support the same locale as the OS, the user would end up with all of my content based on the whichever locale they choose in-game, but anything provided via these localization calls would use their OS locale.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Nov 7, 2019
@aaronfranke
Copy link
Member

Since this PR requires further discussion, you should open a proposal which explains example use cases and how this approach will solve the problem.

This PR is not good as-is due to the above comments about STL, so I'll close this. After a proposal is discussed and approved, this can be re-opened, or you can open a new PR.

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