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

Print some more helpful debug messages #56

Merged
merged 7 commits into from Aug 4, 2018
Merged

Print some more helpful debug messages #56

merged 7 commits into from Aug 4, 2018

Conversation

swissspidy
Copy link
Member

See #46.

@swissspidy swissspidy changed the title [WIP] Print some more helpful debug messages Print some more helpful debug messages Jul 31, 2018
@@ -300,6 +319,8 @@ protected function makepot() {

// Add existing strings first but don't keep headers.
if ( $this->merge ) {
WP_CLI::debug( sprintf( 'Mergining with existing POT file: %s', $this->merge ), 'make-pot' );
Copy link
Member

Choose a reason for hiding this comment

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

There's a typo here: Mergining => Merging


WP_CLI::debug( sprintf( 'Destination: %s', $this->destination ), 'make-pot' );
if ( isset( $this->merge ) && ! file_exists( $this->merge ) ) {
WP_CLI::warning( sprintf( 'POT file to merge with does not exist: %s', $this->merge ) );
Copy link
Member

Choose a reason for hiding this comment

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

Typo: with => which

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems correct to me: "The POT file you want to merge the new translations with(,) does not exist".

Perhaps there's a less ambiguous sentence though.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I read it in a different way: "Here is a reference to the POT file we wanted to merge but which does not exist: ...".

Some random ideas for an alternative...

  • "Merge target does not exist: ..."
  • "POT file to be merged does not exist: ..."
  • "Invalid POT file set as merge target: ..."
  • "File to merge is not valid: ..."
  • "Invalid file provided to --merge: ..."

$translations_count = count( $this->translations );

if ( 1 === $translations_count ) {
WP_CLI::debug( sprintf( 'Extracted %d string', count( $this->translations ) ), 'make-pot' );
Copy link
Member

Choose a reason for hiding this comment

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

Reuse $translations_count here

if ( 1 === $translations_count ) {
WP_CLI::debug( sprintf( 'Extracted %d string', count( $this->translations ) ), 'make-pot' );
} else {
WP_CLI::debug( sprintf( 'Extracted %d strings', count( $this->translations ) ), 'make-pot' );
Copy link
Member

Choose a reason for hiding this comment

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

Reuse $translations_count here

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move over the Inflector class from wp-cli/scaffold-package for strings like this.
You could then do something along these lines:

WP_CLI::debug( sprintf(
   'Extracted %d %s',
   $translations_count,
   Utils\maybe_pluralize( 'string', $translations_count )
), 'make-pot' );

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Only now saw this. The same applies to the language command as well, so 👍

@swissspidy swissspidy requested a review from a team August 3, 2018 15:30
@schlessera schlessera added this to the 0.1.0 milestone Aug 4, 2018
@schlessera schlessera merged commit fc28fd2 into master Aug 4, 2018
@schlessera schlessera deleted the 46-debug branch August 4, 2018 05:13
schlessera added a commit that referenced this pull request Jan 6, 2022
Print some more helpful debug messages
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.

None yet

2 participants