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

Faulty core fixes: switch/case indentation #982

Closed
jrfnl opened this Issue Jun 18, 2017 · 25 comments

Comments

Projects
None yet
6 participants
@jrfnl
Contributor

jrfnl commented Jun 18, 2017

Summary:

There are a number of places in WP core which use - very large - switch/case constructs which are incorrectly indented.
For these, the current fixer seems to create a worse inconsistency in the indentations for:

  • break - indentation not corrected
  • docblocks - indentation of the first line corrected, but not the rest of the docblock.
  • escaping out of PHP

Most likely cause: Generic.WhiteSpace.ScopeIndent

I've seen other bugs with this sniff before and it may actually be simpler to pre-indent the case statement manually before running phpcbf than to fix this sniff.
Alternatively, turning that particular sniff off for the fixer might also save some grief.

Example:

File: various files across the board src/wp-admin/comment.php to name just one, src/wp-admin/edit-tags.php for another.

Example snippet:

switch ( $a ) {
case 'a':
	$b = 1;
	break;

case 'b':
	$b = 2;
	/**
	 * A comment about a hook.
	 */
	apply_filter( 'something', $b );
	break;

case 'a':
	$b = 3;
?>
	<input name="something" value=<?php echo $b; ?>
<?php
	$b = 4;
	break;

default:
	$b = 5;
	break;
}

Fixed as:

switch ( $a ) {
	case 'a':
		$b = 1;
	break;

	case 'b':
		$b = 2;
		/**
	 * A comment about a hook.
	 */
		apply_filter( 'something', $b );
	break;

	case 'a':
		$b = 3;
		?>
		<input name="something" value=<?php echo $b; ?>
	<?php
	$b = 4;
	break;

	default:
		$b = 5;
	break;
}

Expected fix:

switch ( $a ) {
	case 'a':
		$b = 1;
		break;
	
	case 'b':
		$b = 2;
		/**
		 * A comment about a hook.
		 */
		apply_filter( 'something', $b );
		break;
	
	case 'a':
		$b = 3;
		?>
		<input name="something" value=<?php echo $b; ?>
		<?php
		$b = 4;
		break;
	
	default:
		$b = 5;
		break;
}
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 3, 2017

Contributor

Now the standard for break indentation in switch control structures has been added to the handbook, I've looked into whether there was an upstream sniff we can use to cover and auto-fix this.

The PSR2.ControlStructures.SwitchDeclaration sniff would cover this, however it does more than we "need", though the extra checks included aren't necessarily a bad thing.

Error messages in the sniff:

  • NotLower: switch/case/default keyword must be lowercase => Already covered by Generic.PHP.LowerCaseKeyword.Found
  • SpacingAfterCase: CASE keyword must be followed by a single space
  • SpaceBeforeColon: There must be no space before the colon in a case/default statement
  • BodyOnNextLine: The case/default body must start on the line following the statement
  • BreakNotNewLine: Terminating statement must be on a line by itself (=break/return) => Already covered by Generic.Formatting.DisallowMultipleStatements
  • BreakIndent: Terminating statement must be indented to the same level as the CASE body (=break/return) <= This is the one we need.
  • WrongOpener: Case/default statements must be defined using a colon
  • TerminatingComment: There must be a comment when fall-through is intentional in a non-empty case body

Aside from the The case/default body must start on the line following the statement message, I think they would all be good additions to WPCS.

I propose adding this sniff to WordPress-Core, excluding error codes which would lead to duplicate messages and possibly excluding the BodyOnNextLine one if others agree and it would not lead to fixer conflicts.

The WrongOpener and TerminatingComment error codes are more best practice/bug prevention kind of checks, but I don't think it would be a bad thing to leave them enabled in the Core sniffs in this case.

Note: Adding this sniff would not (yet) solve the docblock/escaping out of PHP indentation issues. Those are probably issues with the Generic.WhiteSpace.ScopeIndent sniff and would need to be looked at separately.

The alternative would be to add a WPCS native sniff to only check the indentation of switch control structures. In that case, the docblock/escaping out of PHP indentation issues should be covered by that sniff as well.

Opinions ?

/cc @pento

Contributor

jrfnl commented Jul 3, 2017

Now the standard for break indentation in switch control structures has been added to the handbook, I've looked into whether there was an upstream sniff we can use to cover and auto-fix this.

The PSR2.ControlStructures.SwitchDeclaration sniff would cover this, however it does more than we "need", though the extra checks included aren't necessarily a bad thing.

Error messages in the sniff:

  • NotLower: switch/case/default keyword must be lowercase => Already covered by Generic.PHP.LowerCaseKeyword.Found
  • SpacingAfterCase: CASE keyword must be followed by a single space
  • SpaceBeforeColon: There must be no space before the colon in a case/default statement
  • BodyOnNextLine: The case/default body must start on the line following the statement
  • BreakNotNewLine: Terminating statement must be on a line by itself (=break/return) => Already covered by Generic.Formatting.DisallowMultipleStatements
  • BreakIndent: Terminating statement must be indented to the same level as the CASE body (=break/return) <= This is the one we need.
  • WrongOpener: Case/default statements must be defined using a colon
  • TerminatingComment: There must be a comment when fall-through is intentional in a non-empty case body

Aside from the The case/default body must start on the line following the statement message, I think they would all be good additions to WPCS.

I propose adding this sniff to WordPress-Core, excluding error codes which would lead to duplicate messages and possibly excluding the BodyOnNextLine one if others agree and it would not lead to fixer conflicts.

The WrongOpener and TerminatingComment error codes are more best practice/bug prevention kind of checks, but I don't think it would be a bad thing to leave them enabled in the Core sniffs in this case.

Note: Adding this sniff would not (yet) solve the docblock/escaping out of PHP indentation issues. Those are probably issues with the Generic.WhiteSpace.ScopeIndent sniff and would need to be looked at separately.

The alternative would be to add a WPCS native sniff to only check the indentation of switch control structures. In that case, the docblock/escaping out of PHP indentation issues should be covered by that sniff as well.

Opinions ?

/cc @pento

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 3, 2017

Contributor

FYI: I've also opened an upstream issue regarding the docblock/inline HTML indentation: squizlabs/PHP_CodeSniffer#1539

Contributor

jrfnl commented Jul 3, 2017

FYI: I've also opened an upstream issue regarding the docblock/inline HTML indentation: squizlabs/PHP_CodeSniffer#1539

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Jul 3, 2017

Member

SpacingAfterCase: CASE keyword must be followed by a single space
SpaceBeforeColon: There must be no space before the colon in a case/default statement

Do these two not contradict each other?

Member

GaryJones commented Jul 3, 2017

SpacingAfterCase: CASE keyword must be followed by a single space
SpaceBeforeColon: There must be no space before the colon in a case/default statement

Do these two not contradict each other?

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Jul 3, 2017

Member
  1. SpacingAfterCase: CASE keyword must be followed by a single space
  2. SpaceBeforeColon: There must be no space before the colon in a case/default statement
    Do these two not contradict each other?

No they don't, here's some examples:

case 'a':
//  ^   ^
//  1   2
case'b': // is a violation of `SpacingAfterCase`
case 'c' : // is a violation of `SpaceBeforeColon`
case'd' : // is a violation of both `SpacingAfterCase` and `SpaceBeforeColon`
Member

ntwb commented Jul 3, 2017

  1. SpacingAfterCase: CASE keyword must be followed by a single space
  2. SpaceBeforeColon: There must be no space before the colon in a case/default statement
    Do these two not contradict each other?

No they don't, here's some examples:

case 'a':
//  ^   ^
//  1   2
case'b': // is a violation of `SpacingAfterCase`
case 'c' : // is a violation of `SpaceBeforeColon`
case'd' : // is a violation of both `SpacingAfterCase` and `SpaceBeforeColon`
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 3, 2017

Contributor

@GaryJones No, they don't - between the case and the : you always have a value or condition ;-)

switch ( $a ) {
    case 'b':
        // One space between `case` and `'b'`.
        // No space between `'b'` and `:`.
        break;
}
Contributor

jrfnl commented Jul 3, 2017

@GaryJones No, they don't - between the case and the : you always have a value or condition ;-)

switch ( $a ) {
    case 'b':
        // One space between `case` and `'b'`.
        // No space between `'b'` and `:`.
        break;
}
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 3, 2017

Contributor

@ntwb Ah, sorry, I didn't see your comment, should have refreshed the page before replying.
@GaryJones What @ntwb said ;-)

Contributor

jrfnl commented Jul 3, 2017

@ntwb Ah, sorry, I didn't see your comment, should have refreshed the page before replying.
@GaryJones What @ntwb said ;-)

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Jul 3, 2017

Member

Ah, of course - I was forgetting the value. I apparently don't use switch statements often enough!

I've got no objection to these being pushed through WordPress-Core, but it would be good to get them documented in the Handbook as well.

Member

GaryJones commented Jul 3, 2017

Ah, of course - I was forgetting the value. I apparently don't use switch statements often enough!

I've got no objection to these being pushed through WordPress-Core, but it would be good to get them documented in the Handbook as well.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 3, 2017

Contributor

I've got no objection to these being pushed through WordPress-Core, but it would be good to get them documented in the Handbook as well.

Ok, so I've dug into this a bit deeper.

Current practices used in Core

I figured it might make sense to actually see what's currently used in Core to decide which rules to adopt anyhow. The SwitchDeclaration sniff does not create any PHPCS metrics, so we can't see how this has evolved in WP historically.
For now, I've added some metrics code locally and ran the info report (and opened a related upstream issue squizlabs/PHP_CodeSniffer#1541).

The results were both interesting as well as surprising in some aspects:

PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
Blank line at start of case/default:
        0 [3297/3325, 99.16%]
        1 => 28 (0.84%)

Case followed by space:
        1 [3056/3058, 99.93%]
        2 => 2 (0.07%)

Default/case statement followed by colon:
        yes [3325/3327, 99.94%]
        no => 2 (0.06%)

Fall through case:
        no [149/276, 53.99%]
        yes, with comment => 116 (42.03%)
        yes, but without comment => 11 (3.99%)

Keywords: lowercase [3327/3327, 100%]

Space before colon:
        no [2939/3325, 88.39%]
        1 => 365 (10.98%)
        5 => 4 (0.12%)
        4 => 4 (0.12%)
        3 => 4 (0.12%)
        2 => 4 (0.12%)
        8 => 2 (0.06%)
        7 => 1 (0.03%)
        9 => 1 (0.03%)
        6 => 1 (0.03%)

Terminating statement indented one tab from case:
        yes [1687/1873, 90.07%]
        no => 186 (9.93%)

Terminating statement on line by itself:
        yes [1873/1881, 99.57%]
        no => 8 (0.43%)
----------------------------------------------------------------------

Updated proposal

Based on this, I believe we should add the sniff:

  • SpacingAfterCase (99.9% compliance)
  • SpaceBeforeColon (88,3% compliance)
  • BodyOnNextLine (99.2% compliance)
  • BreakIndent: (90.1% compliance)
  • WrongOpener (99.9% compliance)
  • TerminatingComment (86.6% compliance where relevant)

and exclude:

  • NotLower (100% compliance) - duplicate msg
  • BreakNotNewLine (99.6% compliance) - duplicate msg

Handbook updates needed

Based on the rules I mentioned above and what's already covered in a different way by WPCS (and therefore already in the handbook), I believe the only things which would still need to be added to the handbook would be:

  • "There must be no space before the colon in a case statement."
  • "When a case statement statement does not end in a break/return, but deliberately falls through to the next case, it must be so documented with a comment."
  • "There must be no blank line at the start of the code in a case or default statement." - providing we'd choose to add BodyOnNextLine - this is not a hard rule for other control structures, so we may still need to think about this and/or add the same for other control structures in that case for consistency.

Oh - just noticed: the new example which was added to the handbook for the case/break indentation actually uses a space before the colon in the case statement, so this will need to be adjusted. /cc @ntwb

Detailed look at some of the results of the sniff

WrongOpener

WrongOpener would IMHO not need to be documented as code like that would mostly cause a parse error, so wouldn't be acceptable anyway.
I've investigated the two cases where this was flagged:

  • In wp-includes/class-wp-network.php around line 157 a semi-colon is used, which is clearly a typo and needs to be fixed.
  • Same goes for wp-includes/date.php around line 848.

I've opened a ticket in trac with a patch for these: https://core.trac.wordpress.org/ticket/41234

TerminatingComment

I've also looked at the TerminatingComment cases - most are legitimately flagged and could use a comment, a few are quite possibly bugs in the code where the fall-through is unintentional, and a few are situations where there is a control structure within the case and all possible pathways contain a return or break, but there is no return or break at the very end of the case.
We could consider turning this into a warning and for the last part (all pathways contain an exit) this should possibly be reported upstream as a bug.


Side-note: Adding this sniff and making these rules explicit in the handbook, funnily enough fills a void already pointed out in 2013 in some comments on the handbook https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#comment-9283 and https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#comment-9984 /cc @JDGrimes

Contributor

jrfnl commented Jul 3, 2017

I've got no objection to these being pushed through WordPress-Core, but it would be good to get them documented in the Handbook as well.

Ok, so I've dug into this a bit deeper.

Current practices used in Core

I figured it might make sense to actually see what's currently used in Core to decide which rules to adopt anyhow. The SwitchDeclaration sniff does not create any PHPCS metrics, so we can't see how this has evolved in WP historically.
For now, I've added some metrics code locally and ran the info report (and opened a related upstream issue squizlabs/PHP_CodeSniffer#1541).

The results were both interesting as well as surprising in some aspects:

PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
Blank line at start of case/default:
        0 [3297/3325, 99.16%]
        1 => 28 (0.84%)

Case followed by space:
        1 [3056/3058, 99.93%]
        2 => 2 (0.07%)

Default/case statement followed by colon:
        yes [3325/3327, 99.94%]
        no => 2 (0.06%)

Fall through case:
        no [149/276, 53.99%]
        yes, with comment => 116 (42.03%)
        yes, but without comment => 11 (3.99%)

Keywords: lowercase [3327/3327, 100%]

Space before colon:
        no [2939/3325, 88.39%]
        1 => 365 (10.98%)
        5 => 4 (0.12%)
        4 => 4 (0.12%)
        3 => 4 (0.12%)
        2 => 4 (0.12%)
        8 => 2 (0.06%)
        7 => 1 (0.03%)
        9 => 1 (0.03%)
        6 => 1 (0.03%)

Terminating statement indented one tab from case:
        yes [1687/1873, 90.07%]
        no => 186 (9.93%)

Terminating statement on line by itself:
        yes [1873/1881, 99.57%]
        no => 8 (0.43%)
----------------------------------------------------------------------

Updated proposal

Based on this, I believe we should add the sniff:

  • SpacingAfterCase (99.9% compliance)
  • SpaceBeforeColon (88,3% compliance)
  • BodyOnNextLine (99.2% compliance)
  • BreakIndent: (90.1% compliance)
  • WrongOpener (99.9% compliance)
  • TerminatingComment (86.6% compliance where relevant)

and exclude:

  • NotLower (100% compliance) - duplicate msg
  • BreakNotNewLine (99.6% compliance) - duplicate msg

Handbook updates needed

Based on the rules I mentioned above and what's already covered in a different way by WPCS (and therefore already in the handbook), I believe the only things which would still need to be added to the handbook would be:

  • "There must be no space before the colon in a case statement."
  • "When a case statement statement does not end in a break/return, but deliberately falls through to the next case, it must be so documented with a comment."
  • "There must be no blank line at the start of the code in a case or default statement." - providing we'd choose to add BodyOnNextLine - this is not a hard rule for other control structures, so we may still need to think about this and/or add the same for other control structures in that case for consistency.

Oh - just noticed: the new example which was added to the handbook for the case/break indentation actually uses a space before the colon in the case statement, so this will need to be adjusted. /cc @ntwb

Detailed look at some of the results of the sniff

WrongOpener

WrongOpener would IMHO not need to be documented as code like that would mostly cause a parse error, so wouldn't be acceptable anyway.
I've investigated the two cases where this was flagged:

  • In wp-includes/class-wp-network.php around line 157 a semi-colon is used, which is clearly a typo and needs to be fixed.
  • Same goes for wp-includes/date.php around line 848.

I've opened a ticket in trac with a patch for these: https://core.trac.wordpress.org/ticket/41234

TerminatingComment

I've also looked at the TerminatingComment cases - most are legitimately flagged and could use a comment, a few are quite possibly bugs in the code where the fall-through is unintentional, and a few are situations where there is a control structure within the case and all possible pathways contain a return or break, but there is no return or break at the very end of the case.
We could consider turning this into a warning and for the last part (all pathways contain an exit) this should possibly be reported upstream as a bug.


Side-note: Adding this sniff and making these rules explicit in the handbook, funnily enough fills a void already pointed out in 2013 in some comments on the handbook https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#comment-9283 and https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#comment-9984 /cc @JDGrimes

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Jul 4, 2017

Member

I'm a little surprised at the space before colon stats, I thought there would be more cases where spaces have been used for alignment:

Here's some examples:

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/setup-config.php#L341-L348

case 'AUTH_KEY'         :
case 'SECURE_AUTH_KEY'  :
case 'LOGGED_IN_KEY'    :
case 'NONCE_KEY'        :
case 'AUTH_SALT'        :
case 'SECURE_AUTH_SALT' :
case 'LOGGED_IN_SALT'   :
case 'NONCE_SALT'       :

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/comment.php#L75-L78

case 'delete'  :
case 'approve' :
case 'trash'   :
case 'spam'    :

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/comment.php#L238-L244

case 'deletecomment'    :
case 'trashcomment'     :
case 'untrashcomment'   :
case 'spamcomment'      :
case 'unspamcomment'    :
case 'approvecomment'   :
case 'unapprovecomment' :
Member

ntwb commented Jul 4, 2017

I'm a little surprised at the space before colon stats, I thought there would be more cases where spaces have been used for alignment:

Here's some examples:

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/setup-config.php#L341-L348

case 'AUTH_KEY'         :
case 'SECURE_AUTH_KEY'  :
case 'LOGGED_IN_KEY'    :
case 'NONCE_KEY'        :
case 'AUTH_SALT'        :
case 'SECURE_AUTH_SALT' :
case 'LOGGED_IN_SALT'   :
case 'NONCE_SALT'       :

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/comment.php#L75-L78

case 'delete'  :
case 'approve' :
case 'trash'   :
case 'spam'    :

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/comment.php#L238-L244

case 'deletecomment'    :
case 'trashcomment'     :
case 'untrashcomment'   :
case 'spamcomment'      :
case 'unspamcomment'    :
case 'approvecomment'   :
case 'unapprovecomment' :
@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Jul 4, 2017

Member

Oh - just noticed: the new example which was added to the handbook for the case/break indentation actually uses a space before the colon in the case statement, so this will need to be adjusted. /cc @ntwb

Done 👍

Member

ntwb commented Jul 4, 2017

Oh - just noticed: the new example which was added to the handbook for the case/break indentation actually uses a space before the colon in the case statement, so this will need to be adjusted. /cc @ntwb

Done 👍

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 9, 2017

Contributor

@WordPress-Coding-Standards/wpcs-collaborators I'd like some opinions on the proposed additions to the handbook.

Once that's been decided on, the new sniff could be added - with or without exclusions - to fix the issue.

Contributor

jrfnl commented Jul 9, 2017

@WordPress-Coding-Standards/wpcs-collaborators I'd like some opinions on the proposed additions to the handbook.

Once that's been decided on, the new sniff could be added - with or without exclusions - to fix the issue.

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jul 9, 2017

Member

"There must be no space before the colon in a case statement."

Yes.

"There must be no blank line at the start of the code in a case or default statement."

Yes.

"When a case statement statement does not end in a break/return, but deliberately falls through to the next case, it must be so documented with a comment."

Just to clarify, this would only apply when there's a body to the case statement? A series of empty cases wouldn't require a comment for each.

Member

pento commented Jul 9, 2017

"There must be no space before the colon in a case statement."

Yes.

"There must be no blank line at the start of the code in a case or default statement."

Yes.

"When a case statement statement does not end in a break/return, but deliberately falls through to the next case, it must be so documented with a comment."

Just to clarify, this would only apply when there's a body to the case statement? A series of empty cases wouldn't require a comment for each.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 9, 2017

Contributor

Just to clarify, this would only apply when there's a body to the case statement? A series of empty cases wouldn't require a comment for each.

Correct.

// This would be fine.
switch ( $foo ) {
	case 'a':
	case 'b':
		break;
}

// This would be fine too.
switch ( $foo ) {
	case 'a':
	case 'b':
		return;
}

// This would require a comment for `case 'a'`.
switch ( $foo ) {
	case 'a':
		echo $foo;
	case 'b':
		return;
}
Contributor

jrfnl commented Jul 9, 2017

Just to clarify, this would only apply when there's a body to the case statement? A series of empty cases wouldn't require a comment for each.

Correct.

// This would be fine.
switch ( $foo ) {
	case 'a':
	case 'b':
		break;
}

// This would be fine too.
switch ( $foo ) {
	case 'a':
	case 'b':
		return;
}

// This would require a comment for `case 'a'`.
switch ( $foo ) {
	case 'a':
		echo $foo;
	case 'b':
		return;
}
@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jul 9, 2017

Member

Okay, they've all been added to the handbook.

Member

pento commented Jul 9, 2017

Okay, they've all been added to the handbook.

jrfnl added a commit that referenced this issue Jul 9, 2017

Add `PSR2.ControlStructures.SwitchDeclaration` to the `WordPress-Core…
…` ruleset

As proposed in #982 (comment)

Also adds the newly added section + rules from the handbook which came out of the discussion in #982.

Partially fixes 982
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 9, 2017

Contributor

Thanks @pento.

I've pulled the rule addition now as PR #1021.

Note: This doesn't completely fix the issue yet. There is an open issue upstream regarding the docblock/inline HTML indentation: squizlabs/PHP_CodeSniffer#1539

Contributor

jrfnl commented Jul 9, 2017

Thanks @pento.

I've pulled the rule addition now as PR #1021.

Note: This doesn't completely fix the issue yet. There is an open issue upstream regarding the docblock/inline HTML indentation: squizlabs/PHP_CodeSniffer#1539

@JDGrimes

This comment has been minimized.

Show comment
Hide comment
@JDGrimes

JDGrimes Jul 9, 2017

Contributor

I fully agree with the other two additions to the handbook, but I would note that personally I prefer to add a blank line at the start of any control structure block that is more than one or two lines long. I find this more readable. But I do realize that core usually doesn't follow this pattern, so I'll understand if core is going to standardize on no blank lines at the start of control structures. (And even I usually omit the blank line at the start of a case block, I only add them for other control structures.) I just thought that I would make my voice heard.

Contributor

JDGrimes commented Jul 9, 2017

I fully agree with the other two additions to the handbook, but I would note that personally I prefer to add a blank line at the start of any control structure block that is more than one or two lines long. I find this more readable. But I do realize that core usually doesn't follow this pattern, so I'll understand if core is going to standardize on no blank lines at the start of control structures. (And even I usually omit the blank line at the start of a case block, I only add them for other control structures.) I just thought that I would make my voice heard.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 9, 2017

Contributor

I prefer to add a blank line at the start of any control structure block that is more than one or two lines long. I find this more readable.

Same here, especially as a dyslectic. Whitespace can make a lot of difference in code readability.

Contributor

jrfnl commented Jul 9, 2017

I prefer to add a blank line at the start of any control structure block that is more than one or two lines long. I find this more readable.

Same here, especially as a dyslectic. Whitespace can make a lot of difference in code readability.

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jul 10, 2017

Member

Hrrm, that's an excellent point, it can improve readability. It's really not used very often in Core, though.

Given variations of the following command, here are the stats I got.

src$ ag --php --stats '^(\t+)(if|while|for|do|switch|case|foreach|[a-z]* ?function|(} )?else).*\n\n(\1\t.*\n){1}\1[^\t]' wp-includes/ wp-admin/
Block Length No Blank Blank
1 8924 4
2 2447 8
3 1112 4
4 889 8
5 670 7
6 462 1
7 381 2
8 237 9
9 235 9
10+ 663 23

It's not at all common, though it does get slight more use as blocks get longer, which is to be expected.

I'm not really excited about any sort of hard rule depending on number of lines, though. Perhaps the best option is to soften the language a bit.

The first line of a new code block should not be blank, unless it clearly improves readability. The exception to this is the block inside a case statement, which must never start with a blank line.

Member

pento commented Jul 10, 2017

Hrrm, that's an excellent point, it can improve readability. It's really not used very often in Core, though.

Given variations of the following command, here are the stats I got.

src$ ag --php --stats '^(\t+)(if|while|for|do|switch|case|foreach|[a-z]* ?function|(} )?else).*\n\n(\1\t.*\n){1}\1[^\t]' wp-includes/ wp-admin/
Block Length No Blank Blank
1 8924 4
2 2447 8
3 1112 4
4 889 8
5 670 7
6 462 1
7 381 2
8 237 9
9 235 9
10+ 663 23

It's not at all common, though it does get slight more use as blocks get longer, which is to be expected.

I'm not really excited about any sort of hard rule depending on number of lines, though. Perhaps the best option is to soften the language a bit.

The first line of a new code block should not be blank, unless it clearly improves readability. The exception to this is the block inside a case statement, which must never start with a blank line.

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Jul 10, 2017

Member

unless it clearly improves readability.

That's open to wildly different interpretation, and that's not what we should be encouraging in the Handbook IMO.

Member

GaryJones commented Jul 10, 2017

unless it clearly improves readability.

That's open to wildly different interpretation, and that's not what we should be encouraging in the Handbook IMO.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 10, 2017

Contributor

Practical point:
What about leaving the "blank line at top" part out of the handbook as well as the ruleset for now and moving it to another issue to be discussed there ?

It's a discussion which is unrelated to this particular issue (switch layout not being fixed correctly) - and as far as I can see at this moment - not one which is currently causing fixer conflicts -, so we can leave deciding upon whether to have a soft/hard rule about blank lines and if so, the wording and the sniffs we'd need, for a later point in time.

Contributor

jrfnl commented Jul 10, 2017

Practical point:
What about leaving the "blank line at top" part out of the handbook as well as the ruleset for now and moving it to another issue to be discussed there ?

It's a discussion which is unrelated to this particular issue (switch layout not being fixed correctly) - and as far as I can see at this moment - not one which is currently causing fixer conflicts -, so we can leave deciding upon whether to have a soft/hard rule about blank lines and if so, the wording and the sniffs we'd need, for a later point in time.

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jul 11, 2017

Member

Okay, I missed that it wasn't required for this fix. 🙂

I've removed the Blank Lines section from the handbook, pending further discussion.

Member

pento commented Jul 11, 2017

Okay, I missed that it wasn't required for this fix. 🙂

I've removed the Blank Lines section from the handbook, pending further discussion.

jrfnl added a commit that referenced this issue Jul 11, 2017

Add `PSR2.ControlStructures.SwitchDeclaration` to the `WordPress-Core…
…` ruleset

As proposed in #982 (comment)

Also adds the newly added section + rules from the handbook which came out of the discussion in #982.

Partially fixes 982
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 11, 2017

Contributor

@pento Sounds good to me. I've updated the open PR to reflect this now.

Contributor

jrfnl commented Jul 11, 2017

@pento Sounds good to me. I've updated the open PR to reflect this now.

@JDGrimes JDGrimes closed this in #1021 Jul 11, 2017

@jrfnl jrfnl reopened this Jul 11, 2017

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 11, 2017

Contributor

Reopened for the scope indent parts of the original report - the auto-close wasn't supposed to get triggered by the PR.

Contributor

jrfnl commented Jul 11, 2017

Reopened for the scope indent parts of the original report - the auto-close wasn't supposed to get triggered by the PR.

@IuliaCazan

This comment has been minimized.

Show comment
Hide comment
@IuliaCazan

IuliaCazan Oct 19, 2017

Just an observation: the rule of SpaceBeforeColon: There must be no space before the colon in a case/default statement kinda breaks the spacing consistency rules. Why is the case structure more special?

From the readability standpoint, I am in favor of spacing and empty lines to make the code much more easy to follow. Consistent rules make the code more easy to understand/implement/extend.

In this case, a more consistent rule would mean to treat the colon in the same way in all the contexts, either when it's used in an alternative syntax for control structures, or as a ternary operator.

IuliaCazan commented Oct 19, 2017

Just an observation: the rule of SpaceBeforeColon: There must be no space before the colon in a case/default statement kinda breaks the spacing consistency rules. Why is the case structure more special?

From the readability standpoint, I am in favor of spacing and empty lines to make the code much more easy to follow. Consistent rules make the code more easy to understand/implement/extend.

In this case, a more consistent rule would mean to treat the colon in the same way in all the contexts, either when it's used in an alternative syntax for control structures, or as a ternary operator.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Dec 1, 2017

Contributor

The comment indent part will be fixed by the upcoming DocCommentFormat sniff as per #1120.

That leaves the PHP open/close tags and associated indentation. This has been discussed in other issues.
PHPCS needs an anchor point to determine indent and uses the open/close tags for that, so this is a won't fix.

That concludes this issue as far as I'm concerned.

Contributor

jrfnl commented Dec 1, 2017

The comment indent part will be fixed by the upcoming DocCommentFormat sniff as per #1120.

That leaves the PHP open/close tags and associated indentation. This has been discussed in other issues.
PHPCS needs an anchor point to determine indent and uses the open/close tags for that, so this is a won't fix.

That concludes this issue as far as I'm concerned.

@jrfnl jrfnl closed this Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment