Skip to content
This repository has been archived by the owner on Feb 22, 2019. It is now read-only.

Enable POT generation from the Calypso source #13

Merged
merged 3 commits into from
Aug 5, 2016

Conversation

akirk
Copy link
Member

@akirk akirk commented Jul 29, 2016

This enables parsing of babylon parser output which comes through Automattic/xgettext-js#11.

The main changes that need accomodation:

  • Literal is split into subliterals of which we are only interested in StringLiteral
  • the raw property was moved into extra.raw
  • we need to also check for StringLiteral in an innerProp

This is in order to make Automattic/wp-calypso#7152 possible.

@akirk akirk changed the title Parse babylon output Enable POT generation from the Calypso source Aug 1, 2016
@akirk akirk force-pushed the update/new-xgettext-js-version branch from 7606c02 to 8307d29 Compare August 1, 2016 14:42
@akirk
Copy link
Member Author

akirk commented Aug 1, 2016

@deBhal could you help review this and get it ready to merge? Test from Calypso via npm link i18n-calypso from within Automattic/wp-calypso#7152. Thanks!

@akirk
Copy link
Member Author

akirk commented Aug 1, 2016

Maybe long term the file generation code should be moved to xgettext-js, see Automattic/xgettext-js#4.

@akirk akirk force-pushed the update/new-xgettext-js-version branch from 8307d29 to 860b8f5 Compare August 2, 2016 05:15
@deBhal
Copy link
Contributor

deBhal commented Aug 3, 2016

It looks like we're not picking up backticks. ./bin/index.js <( curl https://raw.githubusercontent.com/Automattic/wp-calypso/master/client/my-sites/site-settings/section-export.jsx ) is missing the string from i18n.translate( Visit your site's wp-admin for all your import and export needs. )

@akirk akirk force-pushed the update/new-xgettext-js-version branch from 1ac4912 to 1b51f7f Compare August 3, 2016 05:47
@akirk
Copy link
Member Author

akirk commented Aug 3, 2016

@deBhal thanks for catching this. I'ts now fixed, could you take another look? Thanks!

@akirk akirk force-pushed the update/new-xgettext-js-version branch 2 times, most recently from b2263c6 to 2eb65de Compare August 3, 2016 10:34
@@ -121,7 +111,7 @@ function makeDoubleQuoted( literal ) {

// ES6 string
if ( literal.charAt( 0 ) === '`' ) {
return '"' + literal.substring( 1, literal.length - 1 ).replace( /`/g, '\`' ).replace( /(\\|")/g, '\\$1' ) + '"';
return '"' + literal.substring( 1, literal.length - 1 ).replace( /`/g, '\`' ).replace( /(\\|")/g, '\\$1' ).replace( /\n/g, ' ' ) + '"';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit uncomfortable with the way the replace( /\n/g, ' ' ) introduces a break between what the developer sees and what translators see.

I don't see any translations right now that would go wrong right now ( https://translate.wordpress.com/projects/wpcom/fr/default/?filters%5Bterm%5D=%5Cn&filters%5Buser_login%5D=&filters%5Bstatus%5D=current_or_waiting_or_fuzzy_or_untranslated&filters%5Bview%5D=&filter=Filter&sort%5Bby%5D=priority&sort%5Bhow%5D=desc ), but it would be really hard to spot if something did come up.

If you were really keen to do this, we should add a complementary change to our runtime translate() logic, but I think we should just use the linting rule.

@deBhal
Copy link
Contributor

deBhal commented Aug 4, 2016

Backticks are working nicely now, nice!

Apart from the newline replace, this looks good. I'd suggest pull that out and :shipit: and pop it into a new PR with the matching translate() change if you want to add that functionality.

We'll also need to bump the version to pick up this and #10 after they both land.

@akirk akirk force-pushed the update/new-xgettext-js-version branch from 2eb65de to 6a3aaa6 Compare August 5, 2016 11:34
@akirk akirk merged commit e1d2de8 into master Aug 5, 2016
@akirk akirk deleted the update/new-xgettext-js-version branch August 5, 2016 11:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants