[L10n Tooling: i3] Re-implement download-from-glotpress logic from .py and .swift scripts

[AliSoftware]

Part of the L10n Modernization project paaHJt-2Ib-p2, especially item [i3].

What

This introduces a new ios_download_strings_files_from_glotpress action, which is mostly the re-implementation of:

• The Scripts/update-translations.rb ruby script currently hosted in WPiOS/WCiOS repos
• The Scripts/fix-translation swift script currently hosted in WPiOS/WCiOS repos

PS: Obviously, once this lands and the client repos have migrated to use the future new version of the toolkit including that action and start using it, we will completely delete the scripts it replaces from the client repos' Scripts/ folders.

Handling invalid files and empty translations

I've decided to only warn (using a UI.error) about the potential case of having empty translations in downloaded exports — as opposed to trying to fix it ourselves like the previous Scripts/fix-translation script was trying to do. There are multiple reasons for this choice:

1. The issue that led us to have some empty translations ("key" = "";) in exports in the past seems to have been resolved a long time ago and not be present anymore, at least to my experience and smoke checks in our Mag16 locales
2. The logic to check if there were any empty translations remaining after the fix seemed to assume the .strings files (downloaded from GlotPress then fixed) were in UTF16 (given the bytes used on the grep), while in practice all the downloaded files are all in UTF-8 nowadays (find WordPress/Resources -name 'Localizable.strings' -exec file {} \; in WPiOS only lists the one from en.lproj, aka the original, to be UTF16, and all others downloaded from GlotPress to be UTF8). Which shows how the check was outdated and not even checking correctly what we expected it to fix (as the grep would not find those UTF16 bytes in the UTF-8 files anyway)
3. The fix back in the way it was implemented by Scripts/fix-translations consisted in replacing the empty "key" = ""; with "key" = "key";; but given we want to be open to the possibility of (and start actually adopting) using keys that are different to the English copy (see Project Thread explanations), this would not have been the correct fix anymore, as that would not have been the correct fallback anymore for those cases.
4. A potential fix would have instead be to replace lines matching /= "";$/ with empty lines, or comment those lines out, so that the fallback would instead be to use the value: parameter from NSLocalizedString(_ key:, value:, comment:) in the codebase (which would be the English copy, as opposed to the key). But manipulating the textual .strings file in that way starts raising risks and issues when trying to deal with text encodings (and GlotPress serves those files as application/octet-stream, without explicitly exposing the actual encoding used, even if by experience it looks like UTF8 but we shouldn't make such an assumption if we want our code to be resilient)

Given that the bug seems to have been fixed since a long time I figured better avoid trying to fix something that does not exist — and risk messing things up rather than improving them — and instead just detect in case it reappears one day. If we end up seeing it reappear, it will always be time to try to implement a workaround in the action's code, but… YAGNI.

What's next

While this implements most of what's needed to cover item [i3] of the plan (paaHJt-2Ib-p2), we will still need to add another action on top of this one (in a separate, future PR) to re-implement the logic from Scripts/extract-framework-translations.swift in a separate fastlane action in order to finally complete [i3] (and [i6]). Most of the logic to do that last step is already present in the ios_l10n_helper so that future PR should be straightforward though.

[mokagio]

Took the liberty of adding this to the Localization Toolchain Modernization & Fixes project.

[mokagio]

Looking good so far 👍 Also easier to follow than the original scripts. Looking forward to have everything centralized here.

[AliSoftware]

@mokagio This should now be ready for review 🎉

[AliSoftware]

This is the exception raised by UI.user_error!.

PS: Note that we can't really use allow(FastlaneCore::UI).to receive(:user_error!) { |m| … } here because if we catch and stub those, that won't interrupt the action (i.e. the call here on L9 would not raise if stubbed so the rest of the run method would continue executing).

(And we could do allow(FastlaneCore::UI).to receive(:user_error!) do |e| raise e } to be sure that it still interrupts the code even when stubbed… but then what would be the point of stubbing the method while the real implementation already raises…)

[AliSoftware]

Btw, wdyt of using a lambda like this here (which is basically the Ruby equivalent of an anonymous closure in Swift) so that it allowed me to still keep the # Act and # Assert separate?

Is it readable and worth it? Or too obscure and you'd instead prefer to avoid the lambda and directly use expect { run_described_fastlane_action(…) }.to …… even if that means mixing the "Act" and "Assert" steps together and breaking the usual A/A/A structure of the tests just to be able to catch the exception raised by the "act" part?

[mokagio]

Btw, wdyt of using a lambda like this here...

I think it's a neat trick that doesn't affect the code readability much. Having said that, as much as I like AAA I think it would be okay to break the pattern here to keep the code concise.

Up to your preference.

[AliSoftware]

👍 I'll keep it then 🙂

[mokagio]

I appreciated the prod to test code ratio. Even accounting for the boilerplate required by file system access and Fastlane action and UI, it's still very thorough. Thank you. It makes me trust the code a lot more and will make modifying it in the future easier.

GTG, only left minor non-necessary suggestions.

[mokagio]

Btw, wdyt of using a lambda like this here...

I think it's a neat trick that doesn't affect the code readability much. Having said that, as much as I like AAA I think it would be okay to break the pattern here to keep the code concise.

Up to your preference.