add-textdomain doesn't conform to WP coding standards

[groovecoder]

I used add-textdomain.php from ​http://i18n.svn.wordpress.org/tools/trunk on my plugin code, but it does not include a space before the closing parenthesis of the gettext function call it alters, which violates ​http://codex.wordpress.org/WordPress_Coding_Standards#Space_Usage

[scribu]

Hey groovecoder; it's nice that you want to contribute, but:

1) The add-textdomain.php file is not in the WP Core repository, so posting a patch for it on this trac is not appropriate.

2) Patches that only fix coding standards violations are frowned upon, since they cause code churn, without much to show for it.

[dd32]

1) The add-textdomain.php file is not in the WP Core repository, so posting a patch for it on this trac is not appropriate.

As the i18n trac has never been used, I don't see anything wrong with having that on the core trac.

2) Patches that only fix coding standards violations are frowned upon, since they cause code churn, without much to show for it.

The changes proposed is in a script which alters existing files, the result of the script does not match the WordPress.org coding standards, as such, It's not just adding a missing space to an existing script.

[scribu]

I see. Note that it only partially fixes the problem:

__('Foo');

will now become:

__('Foo', 'some-textdomain' );

[groovecoder]

Right, this script won't fix existing code standard violations, nor should it, IMO. It simply won't introduce new violations into the script.

[groovecoder]

In fact now that I've looked I don't think this script can fix the opening parenthesis without a space, since token_get_all only knows about the '(' character.

Unless we start keeping more state during the token loop like:

if ($prev_token == '(' && $token !== ' ') {
    $token = ' ' . $token;
}
$prev_token = $token

Right before the $this->process_token($token, $inplace) line. But that seems like polluting the script with irrelevant functionality.

[scribu]

I agree. There's a separate script for tidying up PHP code according to WP standards:

​https://github.com/scribu/wp-phptidy

[groovecoder]

Cool, what's the next step here? (I'm too used to github pull requests. ;)

[scribu]

The next step is waiting for someone to commit it to the i18n repo. I have no idea who has commit access there, besides nacin.

[groovecoder]

Is there a way to tag users in tickets here?

[scribu]

You can assign a ticket to someone, but nacin reads every single trac comment, so he'll see this eventually.

[nacin]

Replying to scribu:

You can assign a ticket to someone, but nacin reads every single trac comment, so he'll see this eventually.

By eventually, he means 10 minutes. :-)

I like the idea of keeping more state to make sure we match the coding standard in the document, from comment 5.

[groovecoder]

I like the idea, and I tried it, but it has too many knock-on effects that cause new violations on otherwise well-formatted code like empty array declarations:

$urls = array( );

and multi-line function calls:

$body = wp_remote_retrieve_body(
    wp_remote_get(

We could add conditionals, but that starts to add too much noise to the code. This script's purpose is just to add text domains - but it shouldn't introduce new coding violations so that a dev who took their time to clean the code before adding text domains doesn't have more work to do.

Dev's could and should use ​scribu's script separately to clean code.

[groovecoder]

So are we good with the first patch?

[GaryJ]

21616.2.diff​ is a first pass at this, and is open to suggestions on all aspects.

To check my change was working correctly, I needed some tests. To be able to test sensibly, I neede to be able to just process a string of PHP code, and not individual files, so I needed some refactoring. If this means this diff should go on to a new ticket, which can this fix this ticket, let me know.

The general approach was to make use of the native counter ($index) when looping through the array of tokens, so we can check if the previous token before the closing ) was an instance of whitespace (using coding standards). At the moment, I add a single space to the current token, and strip whatever whitespace was on the end of the previously processed tokens. There's a case where the previous whitespace might have been a line break, not a single space, so this bit could be improved.

With the check of the previous token possible, it becomes a matter of string manipulation to get the desired output.

One thing I did need to change, was to hold on echoing until the file had been completely processed (so that I could remove a previous token during the processing) instead of echo-ing one token at a time. I'm not sure if this will cause any BC behaviour changes.

The new functions are documented, but given a wider ticket scope, I'd refactor process_tokens() even further (and look at making it a protected method).

The tests are simple, but all currently pass.

[GaryJ]

Patch updated to fix incorrect arg when calling process_tokens()

[nacin]

I'm fine with this patch if @ocean90 is.

[groovecoder]

@ocean90 - any movement on this?

[ocean90]

In 36603:

i18n-tools: Respect the coding standards when adding textdomains with add-textdomain.php.

Props GaryJ, groovecoder.
Fixes #21616.