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

separate translation extraction from Po file writing #63

Merged
merged 1 commit into from Aug 6, 2018

Conversation

3 participants
@drzraf
Copy link
Contributor

drzraf commented Jul 31, 2018

split translation extraction from po file writing.
That way child class (or code instantiating this class) can do post processing on $translations array or append new translation (possibly fetched with different extractors), but intended to be stored in the same destination po file

@@ -295,7 +298,7 @@ protected function get_main_file_data() {
*
* @return bool True on success, false otherwise.

This comment has been minimized.

@swissspidy

swissspidy Jul 31, 2018

Member

That's now incorrect with this change.

Instead of making translations a property of the MakePotCommand, I think we can just use $translations = new Translations() and return that object at the end (like you did already).

We could get rid of set_default_headers() and move these ~10 lines into makepot().

Also, since makepot() doesn't really create a POT file anymore, we could call it extract_strings() or something.

@@ -111,10 +111,13 @@ class MakePotCommand extends WP_CLI_Command {
*/
public function __invoke( $args, $assoc_args ) {
$this->handle_arguments( $args, $assoc_args );
if ( ! $this->makepot() ) {
$this->makepot();
if ( !$this->translations ) {

This comment has been minimized.

@swissspidy

swissspidy Jul 31, 2018

Member

Can we keep the space after the !? 🙂

if ( !$this->translations ) {
WP_CLI::warning( 'No translation found' );
}
if ( !PotGenerator::toFile( $this->translations, $this->destination ) ) {

This comment has been minimized.

@swissspidy

swissspidy Jul 31, 2018

Member

Can we keep the space after the !? 🙂

@swissspidy

This comment has been minimized.

Copy link
Member

swissspidy commented Jul 31, 2018

Thanks for this PR, @drzraf! I like where this is heading.

*/
protected function makepot() {
$this->translations = new Translations();
public function extract_strings() {

This comment has been minimized.

@swissspidy

swissspidy Jul 31, 2018

Member

Note that every public method on a class is registered as a subcommand of the command. See https://make.wordpress.org/cli/handbook/commands-cookbook/#required-registration-arguments

I don't think we want this here :-) — protected should work fine for the sake of extensibility.-

This comment has been minimized.

@drzraf

drzraf Jul 31, 2018

Author Contributor

Ouch....
The workaround expressed in #52 (comment) (attempt 4) relies on the fact that instead of inheriting the command, I would be able to instantiate it and grab $translations by calling the [public] extract_strings() method. For handle_arguments it was not such a problem. Isn't the role of phpdoc to register commands?

This comment has been minimized.

@swissspidy

swissspidy Jul 31, 2018

Member

PHPDoc is used for documenting a command and perhaps making aliases. Registration is done via WP_CLI::add_command().

This comment has been minimized.

@drzraf

drzraf Jul 31, 2018

Author Contributor

=> method set to protected

@drzraf drzraf force-pushed the drzraf:extract-wo-save branch from 2c9dc60 to bf71469 Jul 31, 2018

$this->set_default_headers();
$meta = $this->get_meta_data();
$translations->setHeader( 'Project-Id-Version', $meta['name'] . ' ' . $meta['version'] );

This comment has been minimized.

@schlessera

schlessera Jul 31, 2018

Member

Was there a specific reason why this block was inlined instead of remaining in a separate method? I'd consider this is a regression in terms of code quality.

This comment has been minimized.

@swissspidy

swissspidy Jul 31, 2018

Member

I suggested it in an inline comment. I wanted to some refactoring in another PR after this one.

This comment has been minimized.

@drzraf

drzraf Jul 31, 2018

Author Contributor

Because it was requested^^
I restored the function.

This comment has been minimized.

@schlessera

schlessera Jul 31, 2018

Member

Ah, sorry @swissspidy, I missed that comment.

@schlessera schlessera added this to the 0.1.0 milestone Jul 31, 2018

@swissspidy swissspidy requested a review from schlessera Aug 3, 2018

@swissspidy

This comment has been minimized.

Copy link
Member

swissspidy commented Aug 4, 2018

Unfortunately there's now a minor merge conflict because of another recent PR.

@drzraf Would you mind merging master into the branch to resolve these?

Afterwards I'll happily merge this PR :)

Thanks in advance!

split translation extraction from po file writing. Child class (or co…
…nsummer of this class) may do post processing on translation array or append new translation fetched with different extractor, to be store in the same destination po file

@drzraf drzraf force-pushed the drzraf:extract-wo-save branch from 7778c16 to e0bea13 Aug 6, 2018

@drzraf

This comment has been minimized.

Copy link
Contributor Author

drzraf commented Aug 6, 2018

rebased + squashed

@swissspidy swissspidy merged commit eb8d68f into wp-cli:master Aug 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.