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

Generate i18n files for WPAndroid and WPiOS to use #1520

Merged
merged 15 commits into from
Nov 8, 2019

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Oct 30, 2019

Fixes #939

This PR adds 2 scripts to generate translation files for Wordpress-Android (as a strings.xml file) and WordPress-iOS (as a .swift file containing static strings). Those files will be generated and committed in the bundle folder as:

  • bundle/android/strings.xml for android
  • bundle/ios/GutenbergNativeTranslations.swift for iOS

This process will require a script on each app repository so those 2 files are read and included in the glotpress input file and thus the gutenberg mobile translations will be available in glotpress:

Note that only the strings that are in .(native|ios|android).js files are considered and those that are already part of gutenberg glotpress project (web) are excluded as well.

mchowning and others added 2 commits October 28, 2019 13:36
These scripts stopped working because the gutenberg project changed to
only obtain translations from files in the build folder after the
project was built. As a result, gutenberg is not getting translations
for any *.native.js files. This addresses that by:

1. Performing a gutenberg build and obtaining a pot file from
`gutenberg/build/`.

2. Generating a gutenberg-mobile pot file based on the contents of
`./src/*.js` and `./gutenberg/*.native.js`, while also excluding
anything already contained in the gutenberg pot file that was
generated in step 1.
@Tug Tug self-assigned this Oct 30, 2019
@hypest
Copy link
Contributor

hypest commented Nov 4, 2019

👋

I know this is still WIP so, feel free to ignore this question: gutenberg-native.strings seems largely identical to the stings.xml so, I wonder if one or the other will get removed from the PR in favor of a single file.

@Tug Tug requested review from mchowning, hypest and koke November 7, 2019 14:51
@Tug
Copy link
Contributor Author

Tug commented Nov 7, 2019

Hi @hypest, thanks for having a look!
Indeed gutenberg-native.strings is a file that needed to be removed. It's done now :)

@Tug Tug marked this pull request as ready for review November 7, 2019 15:09
let string77 = NSLocalizedString("hello %s", comment: "")
let string78 = NSLocalizedString("cheeseburger", comment: "")
let string79 = NSLocalizedString("%d cat", comment: "")
let string79Plural = NSLocalizedString("%d cats", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to have those grouped so glotpress knows string79Plural is the plural version of string79? Not sure who to ping to answer this 🤔 Any idea @hypest ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have support for plurals yet 😞 wordpress-mobile/WordPress-iOS#6327

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So I guess it's not worth sending those as extra strings for translations as they won't be picked up by glotpress as the plural version of other strings 🤔 Yeah in that case we should probably remove those

const potFileName = process.argv[ 2 ];
const destination = process.argv[ 3 ];
const potFileContent = fs.readFileSync( potFileName );
const swiftOutput = po2Android( potFileContent, process.argv[ 3 ] );
Copy link
Contributor

@mchowning mchowning Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps something like "xmlOutput" here instead 😄

@@ -68,7 +68,7 @@
"prebundle": "yarn i18n-cache:force",
"prebundle:android": "yarn patch-metro-no-file-watch",
"postbundle:android": "yarn un-patch-metro-no-file-watch",
"bundle": "yarn bundle:android && yarn bundle:ios",
"bundle": "yarn bundle:android && yarn bundle:ios && yarn clean:pot && yarn genstrings",
Copy link
Contributor

@mchowning mchowning Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no "gutenberg*.pot" file, then yarn clean:pot will fail and prevent yarn genstrings from executing. I think adding the -f flag to the rm command in yarn clean:pot would be one way to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird it's not failing for me when there is no pot file, I think this might be because of the wildcard?
I can add -f if it fixes it for you 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth moving this to prebundle with the rest of the i18n work

@mchowning
Copy link
Contributor

With these changes running yarn bundle will build gutenberg, which runs npm install inside gutenberg, and the the node_modules directory is generated inside gutenberg and remains after this script is run. For me locally, having the node_modules directory inside gutenberg causes the mobile gutenberg tests (yarn test) to fail. Maybe this is something unique to my local setup, but if it's not, we might want to update this script to clean up after itself so that someone doesn't run yarn bundle and then later not realize why all their tests are failing.

Of course, this probably won't be an issue once we move to the monorepo.

@Tug
Copy link
Contributor Author

Tug commented Nov 8, 2019

Thank you for reviewing @mchowning
I addressed the issues you mentioned, could you have another look :)?

const potFileContent = fs.readFileSync( potFileName );
const swiftOutput = po2Swift( potFileContent, process.argv[ 3 ] );
fs.writeFileSync( destination, swiftOutput );
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what the non-TTY variant is for? I'm not that familiar with node on the cli, and I don't understand why there are two variants

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so the script can work with pipes: cat gutenberg.pot | node bin/po2swift.js.
I started with this approach because it was easier to test, and I wasn't sure which one I was going to use in package.json. It's not needed anymore so we could remove it but I figured it would be a nice addition

Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good. I'd be careful to avoid plurals in native JS files, since I don't think those will work on iOS yet wordpress-mobile/WordPress-iOS#6327

@Tug
Copy link
Contributor Author

Tug commented Nov 8, 2019

@koke thanks for reviewing :)
Indeed they won't work. I'll remove the plurals for iOS and check the ci failing test on android 👍

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.

Fix our i18n process
4 participants