codeigniter4 / translations

System message translations for CodeIgniter4
https://codeigniter4.github.io/translations/
MIT License
197 stars 206 forks source link

[All languages] Test for and remove untranslated keys. #175

Closed sfadschm closed 3 years ago

sfadschm commented 3 years ago

This should probably wait for #173.

This PR adds a new test method testAllIncludedLanguageKeysAreTranslated which scans all defined translations keys and fails if they are strictly the same as the original string in the main repo.

This way, future PR will fail if a maintainer/contributor simply copies the original keys from the main repo in order to satisfy the other tests.

Also, this PR removes all current language keys from all languages that are reported by this test. As the CI4 language loader falls back to en anyways if it does not find a translation key, there is no point in keeping those else than pretending to have complete translations when they aren't:

https://github.com/codeigniter4/CodeIgniter4/blob/fae081f6e9176278e4b0c3e4de756e07c352f746/system/Language/Language.php#L132-L137

Of course there are certain cases and language keys which do not necessarily need to differ from the main repo, e.g., the file size abbreviations (Number.kilobyteAbbr will likely always be kB). These are listed in an $excludeKey array in the test function. For the case when a translated key does not differ from the english string, a new attribute $excludedKeyTranslations has been added to each locales test class, where locale specific exceptions can be added. These two arrays are merged before testing each locale.

sfadschm commented 3 years ago

I also wasn't happy with the static solution but couldn't find any other working one. The setUp method was the solution, thanks :). I rebased and squashed all commits for a prettier commit history.

What is still failing with this version is that AllLocalesTranslationTest also executes the new test function with all locales but does not set up the locales, therefore the locale's exclusions are not loaded and throw errors.

sfadschm commented 3 years ago

@paulbalandan I think we need to update dependencies or such? The current test error was resolved for me locally after composer update.

sfadschm commented 3 years ago

Just thought about it and I'm wondering whether we really need the AllLocalesTranslationTest? Each changed locale would be tested anyways using its own test and AllLocalesTranslationTest does just the same but without calling the classes so that special treatments defined for each language will not come into action.

I would suggest dumping these lines: https://github.com/codeigniter4/translations/blob/c967e65481bcccc06b63f3d6d9262a83529f0fa8/.github/scripts/continuous-integration#L57 https://github.com/codeigniter4/translations/blob/c967e65481bcccc06b63f3d6d9262a83529f0fa8/.github/scripts/continuous-integration#L59-L61

Personally I would keep the message though.

paulbalandan commented 3 years ago

@paulbalandan I think we need to update dependencies or such? The current test error was resolved for me locally after composer update.

Fixed in #181

sfadschm commented 3 years ago

Fine with that, I can rebase after the other PRs are in. I think also #181 would be good to make sure the new test works (it does locally, but who knows :D).

sfadschm commented 3 years ago

Rebased and I think also ready. @lucagrandicelli could you check the removed it keys from Time.php and Migrations.php. Should these stay in the english form or should they be translated? In the first case, I will add them to the it-exceptions, in the latter case, please submit a PR after this is merged.

lucagrandicelli commented 3 years ago

@sfadschm Ok, turns out that on Time.php everything should stay as it is, so i think you can add an exception. On Migrations there are two strings that i missed, while Migrations.namespace Migrations.filename and Migrations.batch must remain in English. I'm gonna make a new PR shortly.

sfadschm commented 3 years ago

Perfect. Added them to the exlude list.

paulbalandan commented 3 years ago

Merging this.