Codecov Report

Merging #262 into master will decrease coverage by 0.24%.
The diff coverage is 7.69%.

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
- Coverage   45.90%   45.65%   -0.25%     
==========================================
  Files          44       44              
  Lines        6915     6959      +44     
==========================================
+ Hits         3174     3177       +3     
- Misses       3741     3782      +41     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f9993a...acf3e39. Read the comment docs.

Thanks for looking at this. I appreciate the effort and have been considering when we would need to use ngettext (apparently, a long time ago). I definitely want to support that.

However, I do not think that the get_plural_index method is sustainable, so I would like a different approach. AFAIR, polib can convert the .po file to a .mo file, which we can pass directly to gettext.GNUTranslations (probably with some io.BytesIO-magic to make it look like a handle). I think that would give us to best of both worlds (namely that we work entirely with po-files in the source tree and trivially pick up changes while still have a future proof handling of indexing).

If you have time and the energy to look at it, you are welcome to take a stab at it as it will be a few days before I have time to sit down and code this.

If we want to use native gettext, we should drop polib.

I have added tests to hit all 4 message catalogs, so I can work on this.

I think it will be easier to use polib.POFile to rewrite the MO file to a temporary file and then load it directly with gettext.GNUTranslations. It would also retain our current "simple" work flow for translators and testers (i.e. they do not have to worry about rebuilding translations).

I made a branch for this at https://github.com/singularity/singularity/tree/use-gettext-for-translations that on-the-fly compiles PO files into MO files and load them, in case you want to base your work on that.

Thanks!

I had already gotten to writing the MO someplace, but not to loading it yet. And I did go with polib in the end - msgfmt.py is not available on my system, so that would be something too complicated for translators to figure out.

We should think about packaging the files for release though instead of rewriting them every time the game starts.

I have tested your branch and I'd prefer to stick to standards, creating a proper LC_MESSAGES directory for each locale to house the translations. This is what experiences translators expect.

How about storing a shasum of the po files to check whether a mo file needs writing? Such an approach would also incur less writing to hard disk, which will extend the lifetimes of SSDs.

Well, we can forget about using polib. The save_as_mofile function omits the plural strings from the mo file, so we lose the translations.

Our best bet for updating the mo files from within the program is a subprocess call to the gettext library on the operating system. This will mean jumping through hoops if we're not on Linux. So, I guess we just have to bite the bullet and require from translators that they run the update script, and include the mo files in the packaging.

Seems like polib is not the culprit. I deleted the plural strings from my translation file, ran utils/gettext-singularity and translated the plural forms with Virtaal, then used Virtaal to generate the .mo file. This will also omit the plural translations in the mo file.

Then I tried poedit in my Windows VM and have the same problem.

Looks like the plural forms not showing in the .mo file is a display in Virtaal - it suddenly started working today with an .mo file generated out of Virtaal. I'll look into generating the files again now and then I'll need to change our calls for the data functions.

@nthykier Do you know off the top of your head which functions added extra modifiers to the _ calls? There is no previous test coverage for them, and they will not work with standard gettext. I don't want to miss any.

Great it started working. :)

I have tried to fix all uses of _ with extra parameters in the master branch now (with enforcement so that new/unfixed usage will raise an exception).

Thanks!

I have now gone through all the translatable strings that have a { in them, that should cover it.

Next step will be to deal with all textdomains and hunt down any strings that need pluralization.

FYI: I have "cherry-picked" some of your fixes to _-calls to master that I overlooked.

OK, some new problems:

1. We can't use message contexts without forcing everybody on Python 3.8 or newer, so we'll have to continue using our old code for data translations and use gettext for UI translations only. Meh.

2. Language switching doesn't work. It refuses to load anything except for the system locale. The official documentation is here: https://docs.python.org/3.8/library/gettext.html#changing-languages-on-the-fly but I have had no luck with it.

I started the locale switching from scratch today and it's working now.

The only thing that's still broken at this point is that it fails with the test suite, so I have to catch a missing key there. It's working fine though when I run the game. I don't like having special code just to make the test suite happy, that smells like a bug to me.

try:
    main_localedir = dirs.get_writable_file_in_dirs('locale', 'i18n')
except KeyError:
    # Catch KeyError: 'i18n'
    # Which happens with the test suite only
    main_localedir = 'singularity/locale'

Have you tried if you can reproduce the issue -s ? By default the game is running without it but the test (effectively) defaults to -s

@nthykier Running ./run_singularity.sh -s without catching the KeyError works fine.

Generally looks good, but it needs a rebase before it can be merged (due to conflicts in merging).

$ git checkout ngettext 
Switched to branch 'ngettext'
Your branch is up to date with 'gc/ngettext'.
$ git merge master
Already up to date.

So, I don't know where you are getting the conflicts from.

I don't like rebasing branches with conflicts that have more than 1 commit in them, because I have to resolve the same conflict again and again as it steps through the commits. It's easier to just use the Squash option when merging.