WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 20 months ago

#21616 reopened defect (bug)

add-textdomain doesn't conform to WP coding standards

Reported by: groovecoder Owned by:
Milestone: WordPress.org Priority: normal
Severity: normal Version:
Component: I18N Keywords:
Focuses: Cc:

Description

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

Attachments (2)

21616.patch (477 bytes) - added by groovecoder 20 months ago.
21616-2.patch (814 bytes) - added by groovecoder 20 months ago.

Download all attachments as: .zip

Change History (15)

groovecoder20 months ago

comment:1 scribu20 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

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.

comment:2 dd3220 months ago

  • Milestone set to WordPress.org
  • Resolution invalid deleted
  • Status changed from closed to reopened

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.

comment:3 scribu20 months ago

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

__('Foo');

will now become:

__('Foo', 'some-textdomain' );
Version 0, edited 20 months ago by scribu (next)

comment:4 groovecoder20 months ago

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

comment:5 groovecoder20 months ago

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.

comment:6 scribu20 months ago

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

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

comment:7 groovecoder20 months ago

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

comment:8 scribu20 months ago

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.

comment:9 groovecoder20 months ago

Is there a way to tag users in tickets here?

comment:10 follow-up: scribu20 months ago

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

comment:11 in reply to: ↑ 10 nacin20 months ago

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.

groovecoder20 months ago

comment:12 groovecoder20 months ago

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.

Last edited 20 months ago by groovecoder (previous) (diff)

comment:13 groovecoder20 months ago

So are we good with the first patch?

Note: See TracTickets for help on using tickets.