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

Add support for pretty-printing PHP files in wp i18n make-php command #396

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sovetski
Copy link

@sovetski sovetski commented Apr 9, 2024

It should resolve this issue: #395

@sovetski sovetski requested a review from a team as a code owner April 9, 2024 19:09
features/makephp.feature Outdated Show resolved Hide resolved
features/makephp.feature Outdated Show resolved Hide resolved
…the end of file and improve test with more entries to test plural one
@ernilambar
Copy link
Member

ernilambar commented Apr 12, 2024

@sovetski FYI: You can run Behat test locally easily using commands like:

WP_CLI_TEST_DBTYPE=sqlite composer behat features/makephp.feature:263

This will run behat test of given file and 263 is the line number of your new Scenario. This will be helpful for quick test in local environment. With using sqlite, you don't have to face hassle for MySQL setup.

@swissspidy swissspidy added this to the 2.6.2 milestone Apr 12, 2024
@sovetski
Copy link
Author

@sovetski FYI: You can run Behat test locally easily using commands like:

WP_CLI_TEST_DBTYPE=sqlite composer behat features/makephp.feature:263

This will run behat test of given file and 263 is the line number of your new Scenario. This will be helpful for quick test in local environment. With using sqlite, you don't have to face hassle for MySQL setup.

Thank you, it helped a lot, now the test pass successfully

image

*
* @param mixed $value The variable you want to export.
* @param int $level The current indentation level.
* @param int $indentation The number of spaces for indentation. Default is 4.
Copy link
Member

Choose a reason for hiding this comment

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

Can we do tabs instead?

Copy link
Author

Choose a reason for hiding this comment

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

I like using tabs for my own projects, but here as it is something which will be used on other developers IDE, I prefer to use spaces, because tabs configuration depends on the configuration of user. A space will be every time a space, but tab not. Some resources not related to this subject but still on the same issue:

https://medium.com/@peey/what-is-the-inconsistent-use-of-tabs-and-spaces-in-indentation-error-and-why-is-it-caused-f3bbb8b2568b
https://stackoverflow.com/questions/5685406/inconsistent-use-of-tabs-and-spaces-in-indentation

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use spaces, because tabs configuration depends on the configuration of user

That's actually an advantage of tabs :-) But I don't wanna start yet another tabs vs spaces discussion here.

I am mainly suggesting using tabs because that's the WordPress coding standard and IMHO we should be consistent with that.

Copy link
Author

Choose a reason for hiding this comment

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

Great, in this case I will try to change it to tabs to respect the standards, thanks!

Comment on lines 185 to 206
private static function pretty_export( $values, $indentation = 2 ) {
$result = '[' . PHP_EOL;
$indent = str_repeat( ' ', $indentation );

foreach ( $values as $key => $value ) {
$result .= $indent . str_repeat( ' ', $indentation ) . "'$key' => ";

if ( is_array( $value ) ) {
$result .= self::pretty_export( $value, $indentation + $indentation );
} elseif ( strpos( $value, "\0" ) !== false ) {
$parts = explode( "\0", $value );
$result .= "'" . implode( "' . \"\\0\" . '", array_map( 'addslashes', $parts ) ) . "'";
} else {
$result .= "'$value'";
}

$result .= ',' . PHP_EOL;
}

$result .= $indent . ']';
return $result;
}
Copy link
Member

Choose a reason for hiding this comment

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

The whole function is a bit hard to grasp. What's with the\0 checks here for example, why do we need that? 🤔

Copy link
Author

@sovetski sovetski Apr 12, 2024

Choose a reason for hiding this comment

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

Without that, instead of getting

'You have %d new message' => 'Du hast %d neue Nachricht' . "\0" . 'Du hast %d neue Nachrichten',

we will get

'You have %d new message' => 'Du hast %d neue NachrichtDu hast %d neue Nachrichten',

You explained it on this article: https://make.wordpress.org/core/2024/02/27/i18n-improvements-6-5-performant-translations/#:~:text=Note%3A%20EOT%20here%20stands%20for%20the%20%E2%80%9CEnd%20of%20Transmission%E2%80%9D%20character%20(U%2B0004%2C%20or%20%22%5C4%22%20in%20PHP).%20It%E2%80%99s%20the%20same%20delimiter%20as%20in%20gettext%20used%20to%20glue%20the%20context%20with%20the%20singular%E2%80%82string.

Copy link
Member

Choose a reason for hiding this comment

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

My question is why is it doing this here and why is this not needed for the existing, non-pretty var_export() method?

@sovetski
Copy link
Author

I don't understand. The command WP_CLI_TEST_DBTYPE=sqlite composer behat features/makephp.feature:263 was working as well, I just removed 1 space and adapted my scenario, I have this error without any explanation:

image

When I add -vvv it returns an unreadable PHP exception. But the returned error output matches the scenario's expected output, so very weird...

@swissspidy
Copy link
Member

When I add -vvv it returns an unreadable PHP exception. But the returned error output matches the scenario's expected output, so very weird...

It sounds like it doesn't match exactly.

When I manually run the wp i18n command in my terminal, I get this output with lots of indentation (8 spaces):

<?php
return [
        'domain' => 'foo-plugin',
        'plural-forms' => 'nplurals=2; plural=(n != 1);',
        'language' => 'de_DE',
        'project-id-version' => 'Foo Plugin',
        'pot-creation-date' => '2018-05-02T22:06:24+00:00',
        'po-revision-date' => '2018-05-02T22:06:24+00:00',
        'messages' => [
                'Foo Plugin' => 'Foo Plugin',
                'Hello' => 'Hallo',
                'You have %d new message' => 'Du hast %d neue Nachricht' . "\0" . 'Du hast %d neue Nachrichten',
        ],
];

@swissspidy swissspidy removed this from the 2.6.2 milestone Jul 29, 2024
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.

3 participants