Ensure that module header fields can be translated #60
Conversation
felixarntz
added
[Type] Bug
felixarntz
requested review from
adamsilverstein,
JustinyAhin,
swissspidy,
ThierryA and
aristath
bin/plugin/commands/translations.js
Outdated
| for await ( const line of rl ) { | ||
| headers.forEach( ( header, index ) => { | ||
| const regex = new RegExp( | ||
| '^(?:[ \t]*<?php)?[ \t/*#@]*' + header + ':(.*)$', |
There was a problem hiding this comment.
Looks like the capturing group can be adjusted to only get valid words.
| '^(?:[ \t]*<?php)?[ \t/*#@]*' + header + ':(.*)$', | |
| '^(?:[ \t]*<?php)?[ \t/*#@]*' + header + ':\s*([\w|\s]*).*$', |
It also removes the need to call trim() on the next iteration.
There was a problem hiding this comment.
Hmm, I get the idea, but there's something off with that regex. I tried to add it, but afterwards no matches were found anymore - note that the fields may contain spaces. If that's crucial, feel free to revise.
There was a problem hiding this comment.
Why not just ^\s*\*\s*${ header }:\s*(.*)$? https://regex101.com/r/BpmfsY/1
It will help to avoid doing an additional cleanup afterwards:
if ( match ) {
- const headerValue = match[ 1 ]
- .replace( /\s*(?:\*\/|\?>).*/, '' )
- .trim();
- if ( headerValue ) {
- headers.splice( index, 1 );
- translationStrings.push( headerValue );
- }
+ translationStrings.push( match[1] );
}There was a problem hiding this comment.
Hmm, can you please try that locally? That suggestion doesn't work for me either.
Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>
…ress/performance into fix/59-module-headers-translatable
|
@mitogh Addressed all your feedback, except #60 (comment) (see comment). |
| @@ -24,10 +24,12 @@ | |||
| "@wordpress/scripts": "^19.0", | |||
| "chalk": "4.1.1", | |||
| "commander": "4.1.0", | |||
| "fast-glob": "^3.2.7", | |||
There was a problem hiding this comment.
@felixarntz, we also need to update the package-lock.json to make sure everybody uses the same version of this dependency.
There was a problem hiding this comment.
I may have been doing something wrong, but when I tried it, the package-lock.json was unchanged. It looks like the same version of this dependency was already previously required via another dependency.
bin/plugin/commands/translations.js
Outdated
| for await ( const line of rl ) { | ||
| headers.forEach( ( header, index ) => { | ||
| const regex = new RegExp( | ||
| '^(?:[ \t]*<?php)?[ \t/*#@]*' + header + ':(.*)$', |
There was a problem hiding this comment.
Why not just ^\s*\*\s*${ header }:\s*(.*)$? https://regex101.com/r/BpmfsY/1
It will help to avoid doing an additional cleanup afterwards:
if ( match ) {
- const headerValue = match[ 1 ]
- .replace( /\s*(?:\*\/|\?>).*/, '' )
- .trim();
- if ( headerValue ) {
- headers.splice( index, 1 );
- translationStrings.push( headerValue );
- }
+ translationStrings.push( match[1] );
}…ders and not use promises.
|
@eugene-manuilov Updated based on your feedback! |
Fixes #59
This PR implements an
npm run translationscommand (via the JS infrastructure introduced in #51), which makes the two module header fieldsModule NameandDescriptiontranslatable by extracting the values from all module files and putting them into a generatedmodule-i18n.phpfile. This file allows wordpress.org to recognize these translations as if they were normally used in the plugin. The PR then uses a similar approach to WordPress core when it comes to the headers themselves in the production code: By callingtranslateit will technically go through the same function, which in combination with the generated file ensures that those are translatable in GlotPress.The implementation of the script is heavily based on a combination of WordPress/gutenberg#5310 and wp-pot/wp-pot@c46b8cb, both of which do in parts similar tasks.
The generated file is
.gitignored, since it doesn't need to be in the repository (it isn't evenrequired anywhere, it just needs to "exist" when the plugin is deployed to wordpress.org).To test the script locally, run
npm run translations.