Allignment of assignment operators may produce hard to read code

[azaozz]

Bug Description

Generally inserting extra white spaces to align = and => works quite well. There are many good examples of it in the WordPress Coding Standards. However the current settings for maxPadding and maxColumn allow for edge cases that may make the code harder to read.

The tl;dr:

• Having a lot of white space in the middle of a line of text makes it harder to read.
• Having a lot of white space in the middle of all lines in a paragraph/block of text makes it much harder to read. This is due to the eyes not being able to follow the lines, and "connect" the left side with the right side of the same line.

Prior art: I think everybody has seen tables of content that list the chapters/sections on the left and page numbers on the right. Many of them have ........... connecting the chapter title to the page number. These dots allow the eyes to follow the lines easily. Example:

Of course a TOC may look like this:

But note how the space between the titles and the page numbers is quite shorter and there are blank lines after each entry. That makes it very easy to read.

Unfortunately after reformatting Core's code with the current default settings in WPCS, there are several edge cases like this:

If you try reading the above example a few times you will probably notice how it becomes increasingly harder to match the array keys to the values.

To fix

The default maxPadding value for Generic.Formatting.MultipleStatementAlignment has to be changed from 40 to something like 10. This seems to be a good amount of white space that prevents the above edge case and fully matches all examples in the WordPress Coding Standards.

Unfortunately it seems there is another bug preventing setting of maxColumn to lower value. Testing with something like 20 doesn't seem to work.

Looking back at when maxPadding = 40 and maxColumn = 60 were chosen:

The value for maxPadding which I've chosen is arbitrary - similar to my notes about maxColumn in #1135 - and could probably use some scrutiny before deciding on the real number to use.

The sniff has been added to the Core ruleset with a maxColumn value of 60.
The 60 is arbitrary.

Additional Context (optional)

Looking at the PHP code in WPCS inself, the maxPadding value used there comes from Squiz and is 12 which is much better. See https://github.com/squizlabs/PHP_CodeSniffer/blob/master/phpcs.xml.dist#L110.

Tested Against develop branch?

[ * ] I have verified the issue still exists in the develop branch of WPCS.

[azaozz]

There are several other edge cases that will get fixed/have better readability by changing the maxPadding and maxColumn values:

(Not really sure what's going on here...)

This looks like another bug. The alignment "breaks" the meaning of "parent" > "child" relation. These are different arrays that are in different code blocks on purpose.

[azaozz]

I understand that "personal preferences" may play a significant role here. Most of the above edge cases can be fixed by either inserting blank lines, or by excluding them from sniffs.

However I was really surprised of how much the long white spaces affected me when working on Core. Started seeing "flying spots" in couple of hours and getting a headache in about 4 hours... Had to count how many rows from the top is the arg I needed, then move to the right and count again to find the value. Doing that few times is fine, doing it 50-60 is.... not.

Here is a small test to try to "experience" this first hand :)

Please read this code for about 10 minutes (that's a long time, I know). Try reading through it, then try reading some random lines. Then read couple of lines somewhere else and come back again.

Now read the values for hooks, dom, nux, keycodes and date. Do you find yourself trying to count the lines to be able to find the values?

[pento]

I'm inclined to agree that the current rule adds too much whitespace in a lot of circumstances. As @azaozz described, it can be quite difficult to figure out which key goes with which value when there's a large gap caused by one or two really long keys.

That said, I'm not a fan of just heavily limiting Generic.Formatting.MultipleStatementAlignment as a solution. For example, doing that at the top of wptexturize() looks like this:

function wptexturize( $text, $reset = false ) {
	global $wp_cockneyreplace, $shortcode_tags;
	static $static_characters      = null,
		$static_replacements       = null,
		$dynamic_characters        = null,
		$dynamic_replacements      = null,
		$default_no_texturize_tags = null,
		$default_no_texturize_shortcodes = null,
		$run_texturize = true,
		$apos          = null,
		$prime         = null,
		$double_prime  = null,
		$opening_quote = null,
		$closing_quote = null,
		$opening_single_quote = null,
		$closing_single_quote = null,
		$open_q_flag          = '<!--oq-->',
		$open_sq_flag         = '<!--osq-->',
		$apos_flag = '<!--apos-->';

	// ...
}

The random mish-mash of indenting just looks messy. However, I think it can be easily cleaned up by adding line breaks to split up the indenting.

function wptexturize( $text, $reset = false ) {
	global $wp_cockneyreplace, $shortcode_tags;
	static $static_characters = null,
		$static_replacements  = null,
		$dynamic_characters   = null,
		$dynamic_replacements = null,

		$default_no_texturize_tags       = null,
		$default_no_texturize_shortcodes = null,

		$run_texturize = true,
		$apos          = null,
		$prime         = null,
		$double_prime  = null,
		$opening_quote = null,
		$closing_quote = null,

		$opening_single_quote = null,
		$closing_single_quote = null,

		$open_q_flag  = '<!--oq-->',
		$open_sq_flag = '<!--osq-->',
		$apos_flag    = '<!--apos-->';

	// ...
}

I think this works similarly well for arrays, too. Looking at your example from _WP_Editors::get_translation(), it would look something like this:

	private static function get_translation() {
		if ( empty( self::$translation ) ) {
			self::$translation = array(
				// Default TinyMCE strings
				'New document' => __( 'New document' ),
				'Formats'      => _x( 'Formats', 'TinyMCE' ),

				'Headings'  => _x( 'Headings', 'TinyMCE' ),
				'Heading 1' => array( __( 'Heading 1' ), 'access1' ),
				'Heading 2' => array( __( 'Heading 2' ), 'access2' ),
				'Heading 3' => array( __( 'Heading 3' ), 'access3' ),
				'Heading 4' => array( __( 'Heading 4' ), 'access4' ),
				'Heading 5' => array( __( 'Heading 5' ), 'access5' ),
				'Heading 6' => array( __( 'Heading 6' ), 'access6' ),

				/* translators: block tags */
				'Blocks'       => _x( 'Blocks', 'TinyMCE' ),
				'Paragraph'    => array( __( 'Paragraph' ), 'access7' ),
				'Blockquote'   => array( __( 'Blockquote' ), 'accessQ' ),
				'Div'          => _x( 'Div', 'HTML tag' ),
				'Pre'          => _x( 'Pre', 'HTML tag' ),
				'Preformatted' => _x( 'Preformatted', 'HTML tag' ),
				'Address'      => _x( 'Address', 'HTML tag' ),

				// ...
			);

			// ...
		}

		// ...
	}

[azaozz]

Yep, the fixed examples look great! :)

I know, it adds another (manual) step but thinking the results are a lot better and are worth it. Also these edge cases are not that common, and having to manually split the affected code blocks is not going to be a lot of work.

To proceed thinking it would be better to change the setting to the default value from Squiz Labs: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/phpcs.xml.dist#L110 including triggering an error when the padding is exceeded (better to stay with warnings). This will detect and throw errors warnings for the current edge cases. Then the blocks can be split and phpcbf run again to adjust the padding. In the future will need to look through the changes before committing to make sure the mess as in the first example above doesn't happen.

Alternatively can leave the setting as-is. Will need to look through the code before every commit and determine "by eye" when the white space is too much, then split the code blocks and rerun phpcbf to adjust the padding. Sounds like this would be harder.

Ideally there would be a way to throw a warning when the MultipleStatementAlignment rule hits the maxPadding value while trying to align the equal signs. Then we'll know where to look :)