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

i18n: Switch to POT generation #7152

Merged
merged 1 commit into from
Aug 5, 2016
Merged

i18n: Switch to POT generation #7152

merged 1 commit into from
Aug 5, 2016

Conversation

akirk
Copy link
Member

@akirk akirk commented Jul 29, 2016

This is dependent on Automattic/xgettext-js#11 (merged) and Automattic/i18n-calypso#13 (merged). It allows us to generate a calypso-strings.pot that include the real source locations of the strings. For example:

#: wp-calypso/client/components/tinymce/plugins/after-the-deadline/core.js:234
#. Proofreading error description
msgid "Spelling"
msgstr ""

Fixes #5172

Side-effects: building calypso-strings.pot doesn't depend on webpack anymore so it's much faster standalone.

cc @rralian @yoavf

@akirk akirk self-assigned this Jul 29, 2016
@akirk akirk force-pushed the add/i18n-pot branch 2 times, most recently from dc906d1 to e4544de Compare July 29, 2016 10:59
@akirk
Copy link
Member Author

akirk commented Aug 1, 2016

@gwwar I adapted the path from your script in #4949 but I don't know if there's another spot that needs a change for this.

@gwwar
Copy link
Contributor

gwwar commented Aug 1, 2016

@akirk see the related pMz3w-5vb-p2 I don't know of any other hook points than this.

@deBhal
Copy link
Contributor

deBhal commented Aug 2, 2016

We'll also need to update our version to pick up the two dependencies we rely on:

  • update xgettext-js version
  • update i18n-calypso version
  • Shrinkwrap

@gwwar
Copy link
Contributor

gwwar commented Aug 2, 2016

@deBhal when updating package.json, please run make shrinkwrap and commit the changes.

@deBhal
Copy link
Contributor

deBhal commented Aug 3, 2016

We're going to have to update the i18n-calypso version as well, but I'll add another checkbox so I don't forget to do it after that. Thanks for the reminder :)

@akirk akirk added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Aug 5, 2016
@akirk
Copy link
Member Author

akirk commented Aug 5, 2016

To test:

  1. make distclean translate
  2. check that a calypso-strings.pot was generated and is valid (open in text editor or PO Edit)
    (3. Creation of the artifact calypso-strings.pot will only happen on master commits)

Thanks!

@@ -37,5 +37,6 @@ env-config.sh
*.db

/calypso-strings.php
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to ignore this anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I left it in to avoid complaints about left-over calypso-strings.php but the CircleCI builds are generated fresh anyway.

@akirk akirk force-pushed the add/i18n-pot branch 2 times, most recently from ea436f0 to ec531a2 Compare August 5, 2016 14:19
translate: node_modules $(CLIENT_CONFIG_FILE)
@CALYPSO_ENV=stage $(BUNDLER)
@CALYPSO_ENV=stage $(LIST_ASSETS) | xargs $(I18N_CALYPSO) --format php --output-file ./calypso-strings.php --array-name calypso_i18n_strings
@CALYPSO_ENV=stage $(I18N_CALYPSO) --format pot --output-file ./calypso-strings.pot $(JS_FILES)
Copy link
Contributor

Choose a reason for hiding this comment

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

So I'm trying to follow the old server/bin/list-assets.js . At the very least, I don't think you need CALYPSO_ENV=stage here anymore, as this was driving which assets file was loaded in the list-assets script. What I haven't been able to determine yet is whether those files were generated for the sole purpose of i18n and can be further cleaned up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting observation, though determining this is possibly outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The assets files are used for looking up bundled script locations by chunk, so they'll need to remain. That being said, there's some change in behavior here then, as previously I18N_CALYPSO was run on the built scripts, and are now being run on the source files. Is that okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting observation, though determining this is possibly outside the scope of this PR.

It's within scope if your changes cause files or logic to become unused 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's part of the reason we are doing this. We want to reference the real string location which was not possible when generating from the built scripts.

@aduth
Copy link
Contributor

aduth commented Aug 5, 2016

LGTM 👍

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 5, 2016
- Adapt artifact script

- Copy the new .pot file to the translate subdir

- Update dependencies and shrinkwrap

Remove list-assets
@akirk akirk merged commit 75ce703 into master Aug 5, 2016
@akirk akirk deleted the add/i18n-pot branch August 5, 2016 14:49
@akirk
Copy link
Member Author

akirk commented Aug 5, 2016

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants