Czech translation

[smrky1]

Added Czech translation for the Base game, Intrigue, Dark Ages, Seaside. Required a couple more changes:

• "cz" is not defined in ISO 639-1. Changing "cz" to "cs", including folders and db files rename.
• ReportLab only supports ISO-8859-1 (Latin1) encoding. Czech language (and others) are not supported in Latin1. Added code in draw.py which loads TTF version of Times Roman font when a non Latin1 language is selected in options
• Added Times Roman TTF requirement for non Latin1 languages to README.md
• Pirate Ship (cz) contains a new +X coins element. Added corresponding icon to image resources.
• Updated CardSorter to use locale based sorting (instead of the previous "strip_accents()" method). Fixes a bug in the previous version which resulted in e.g. Czech language cards to be sorted incorrectly. Correct CZ sorting ("sa" > "sc" > "šb > "ta") vs incorrect ("sa" > "šb" > "sc" > "ta"). Tested in other languages and works OK even in German (de).

[sumpfork]

Thanks! I'll try to get to testing and reviewing this weekend.

[sumpfork]

As a note right away, it does look like you need to pre-commit install and then pre-commit run --all-files.

Also, look at the failing card db compile tests?

[smrky1]

I have run pre-commit run --all-files to lint and test & committed the updates. So far it seems that Compile Card DB / pytest -k "test_languages" runs fine on my machine as well...

[sumpfork]

I have run pre-commit run --all-files to lint and test & committed the updates. So far it seems that Compile Card DB / pytest -k "test_languages" runs fine on my machine as well...

Looks like the set_locale call isn't working in tests: https://github.com/sumpfork/dominiontabs/actions/runs/7857961555/job/21442564732?pr=499

[sumpfork]

I have run pre-commit run --all-files to lint and test & committed the updates. So far it seems that Compile Card DB / pytest -k "test_languages" runs fine on my machine as well...

Looks like the set_locale call isn't working in tests: https://github.com/sumpfork/dominiontabs/actions/runs/7857961555/job/21442564732?pr=499

Yeah, this needs at least the full locale name (language_REGION) like de_DE which we don't provide by default other than in en_US but even there not capitalized properly. It's maybe a good idea to change all the locales to be such actual valid locale strings?

[smrky1]

Seems it is a bit more complicated than that :/ All tests pass OK on my Windows machine (running Python 3.8.18, so same as the test job). The root cause seems to be related to Docker Ubuntu image for the GitHub Action job not having the necessary locales available / set. Together with using incorrect language codes (as you pointed out). Surprisingly the default language codes work on Windows as I wrote above...

I will see if I can figure out some workaround, if not I will revert the locale based sorting back to at least have the rest of the PR working....

Some interesting reading:
https://stackoverflow.com/questions/14547631/python-locale-error-unsupported-locale-setting
https://stackoverflow.com/questions/28405902/how-to-set-the-locale-inside-a-debian-ubuntu-docker-container
https://bobbyhadz.com/blog/locale-error-unsupported-locale-setting-in-python#solving-the-error-in-docker

[smrky1]

I think I managed to figure it out. Required 3 fixes:

1. Adding all necessary locale languages to the Workflows (I added them to compile_card_db.yml and lint_and_test.yml)
2. Also OS dependent locale.setlocale switch in main.py.
3. Additionally I had to rename "nl_du" to "nl_nl" to match a correct language code

All tests should pass now (tested the Workflows on my local machine using act). Please take a look if it is acceptable. Maybe there is a better way, I have to admit this is my first time working with GitHub Workflows. If not, I suggest reverting back to the old non locale based sorting (which will however not sort the cards correctly in all languages) and only commit the Czech translations.

[smrky1]

Added some error handling for when the locale cannot be set (mainly on Linux systems). In that case it will generate a warning and revert to default locale based warning. I think this together with my previous commit is finally a "safe" solution. I also added explanation to Installation section of README.md.

The locale-gen updates in compile_card_db.yml and lint_and_test.yml (from my previous commit) are strictly speaking no longer necessary to pass the tests. However I decided to keep them there to avoid the warnings.

[sumpfork]

This works for me locally now. I'm guessing you maybe need sudo?

[smrky1]

Right - as you say, for whatever reason it works without sudo when using act locally, but not on GitHub servers... Added sudo to generate necessary locales...

[sumpfork]

So I'd like to land this, but I'm getting

dominion_dividers --language=cs
** Warning: --tab-side with 'alternate' implies 2 tabs. Setting --tab-number to 2 **
fish: Job 1, 'dominion_dividers --language=cs' terminated by signal SIGSEGV (Address boundary error)

on my mac. Same in one of the units tests. Will try to figure out what's going on.

[smrky1]

Sorry, I cannot help with that much - do not have Mac :-/ Everything works OK on my Windows 10 and Ubuntu 22.04...

[sumpfork]

This seems to have something to do with not specifying an encoding in the setlocale() call. If I use locale.setlocale(locale.LC_COLLATE, (lang, "UTF-8")) this works on my machine. Otherwise it runs fine through a bunch of comparisons and then suddenly crashes on the second time trying to get a comparison string for Doctor in the cs locale. Default seems to be ISO8859-2for that - really not sure why that would lead to a crash, but can you confirm that this works for sorting for you and change it to UTF-8?

[sumpfork]

Seems to crash on Mac in this form - can you try a 'UTF-8' encoding for comparisons?

[nickv2002]

Thanks for the detailed write up of those options @smrky1

Since we have a dockerfile now, IMO the best option would be 4. We can easily install python-icu inside docker since it's Linux and then have an easy and consistent experience for Mac/Windows (and Linux) users by just directing them to use the containerized version of the CLI.

[sumpfork]

Thanks for the detailed write up of those options @smrky1

Since we have a dockerfile now, IMO the best option would be 4. We can easily install python-icu inside docker since it's Linux and then have an easy and consistent experience for Mac/Windows (and Linux) users by just directing them to use the containerized version of the CLI.

Remember we need to make sure that all of this also works for the online version running in AWS Lambda, which is the access point for most (almost all) users. It's probably not too hard to get that working, but may mean additional work on my side.

I'm leaning towards just making pyicu an optional dependency that we use if present, current behaviour if not.

To note, I'm also travelling for work the next week or so so may be slow to respond.

[nickv2002]

Remember we need to make sure that all of this also works for the online version running in AWS Lambda, which is the access point for most (almost all) users. It's probably not too hard to get that working, but may mean additional work on my side.

Makes sense: you can have Lambdas run arbitrary images, so some configuration changes needed but should work fine. Might be able to install the pyicu at runtime also but it's probably better to pre-build it.

I'm leaning towards just making pyicu an optional dependency that we use if present, current behaviour if not.

That seems reasonable, we can bake it in to the docker image for easy access if folks need it (and avoid all the install hassles that @smrky1 documented above). I'll make the dockerfile change in this branch.

To note, I'm also traveling for work the next week or so so may be slow to respond.

Safe travels!

[sumpfork]

Remember we need to make sure that all of this also works for the online version running in AWS Lambda, which is the access point for most (almost all) users. It's probably not too hard to get that working, but may mean additional work on my side.

Makes sense: you can have Lambdas run arbitrary images, so some configuration changes needed but should work fine. Might be able to install the pyicu at runtime also but it's probably better to pre-build it.

Well, you can't run arbitrary images, just ones based on supported lambda runtimes or close matches. Plus, the function I use is currently an automatically generated one via CDK's python lambda support, not a custom docker lambda. I can switch it, but again, a bit of work :).

So yeah, I'd say

1. support pyicu if we find it's installed (probably just a from icu import Locale in a try/except with a warning that it isn't/can't be imported if that fails)
2. support pyicu locally via docker
3. I'll update the web version to have pyicu installed when I find the time

[smrky1]

OK, agreed! I will take care of 1) in the next few days.

@nickv2002 While you're at it... I checked you dockerfile (being completely new to Docker), and it is not clear to me what line 4 does:
COPY --from=pacodrokad/fonts /fonts /fonts

Is pacodrokad/fonts correct? Seems to me like a path specific to your setup?

[nickv2002]

Well, you can't run arbitrary images, just ones based on supported lambda runtimes or close matches. Plus, the function I use is currently an automatically generated one via CDK's python lambda support, not a custom docker lambda. I can switch it, but again, a bit of work :).

Yeah lots of complexity details there, I was being overly general and you know the implementation details much better. (Just for ref, here the feature I was thinking of and a python example.)

So yeah, I'd say

1. support pyicu if we find it's installed (probably just a from icu import Locale in a try/except with a warning that it isn't/can't be imported if that fails)
2. support pyicu locally via docker
3. I'll update the web version to have pyicu installed when I find the time

Sounds good to me.

@nickv2002 While you're at it... I checked you dockerfile (being completely new to Docker), and it is not clear to me what line 4 does: COPY --from=pacodrokad/fonts /fonts /fonts

Is pacodrokad/fonts correct? Seems to me like a path specific to your setup?

That's a way to get the fonts into the image by copying them from somewhere else (in this case another image) rather than hosting fonts in this repo.

[smrky1]

That's a way to get the fonts into the image by copying them from somewhere else

Might be worth adding to the documentation information on how to set it up correctly then.

When I run docker build . -t dominiontabs according to the instructions, it fails:

 => [internal] load build definition from dockerfile                                                                                                                                                                                                                               0.0s
 => => transferring dockerfile: 806B                                                                                                                                                                                                                                               0.0s
 => ERROR [internal] load metadata for docker.io/pacodrokad/fonts:latest                                                                                                                                                                                                           1.3s
 => [internal] load metadata for docker.io/library/python:3.9-slim                                                                                                                                                                                                                 1.2s
------
 > [internal] load metadata for docker.io/pacodrokad/fonts:latest:
------
dockerfile:4
--------------------
   2 |
   3 |     # get fonts
   4 | >>> COPY --from=pacodrokad/fonts /fonts /fonts
   5 |
   6 |     # Add git for hooks
--------------------
ERROR: failed to solve: pacodrokad/fonts: no match for platform in manifest: not found

[nickv2002]

@smrky1 thanks for that note, it was an arch mismatch (that line is just pulling files, but it still looks for the image of the same arch, I fixed by telling it the specific arch to use). Fixed and merged over here: #503 along with adding python3-icu

• Fix Arch Mismatch identified here
• Adding python3-icu as discussed here

[smrky1]

• Added optional PyICU based sorting + removed the locale related stuff. If PyICU is not found, the script will revert to the default method (using strip_accents()).
• I also updated the README.md documentation (+ created a new page on Wiki with instructions on how to install PyICU)
• To be able to test the PyICU integration in CI, the workflows will use PyICU on Ubuntu runner (but not on Win or macOS)
• I notice that Czech expansions description text had some wrong formatting which caused --expansion_dividers switch to fail with error, this is now fixed

[smrky1]

FYI, I completed the translation of all expansions available in Czech language translation). In the process I found out that the original English set has wrong Peddler description: "+2 Coin" -> should be "+1 Coin". Will rename the pull request title to reflect this.

[sumpfork]

Just getting back to this, will review before I go on my next trip next week.

[sumpfork]

Looks good to me overall, minor comment.

You'll need to rerun precommit run --all-files as I bumped the black version which seems to have changed formatting.

[smrky1]

Only today I found some time to review your feedback. I rerun linting + replaced the implicit sys.modules call to detect icu availability with a helper "have_icu" variable...

[sumpfork]

I just deployed this to the online generator (sumpfork/bgtools_web#40) with instances of all the needed fonts and pyicu for sorting. Took a bunch of work, but I think it's mostly correct now.