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

Fix T_ELSE immediately followed by T_COLON #277

Merged
merged 6 commits into from
Jan 2, 2015

Conversation

nyordanov
Copy link
Contributor

Curently else: throws two errors:

  1. Space after opening control structure is required (the Coding Standards handbook doesn't say anything on this)
  2. No space before opening parenthesis is prohibited (this is just wrong - there is no paranthesis on this line)

This pull request prevents the sniffer from throwing these two errors.


If my interpretation of 1. is considered correct, I could add a fixable error for else :

Curently else: throws two errors:
1) Space after opening control structure is required (the Coding Standards handbook doesn't say anything on this)
2) No space before opening parenthesis is prohibited (this is just wrong - there is no paranthesis on this line)

This commit prevents the sniffer from throwing these two errors.
@JDGrimes
Copy link
Contributor

JDGrimes commented Dec 1, 2014

I prefer the space (else :). I think the motto of the standards is "when in doubt, space it out."

@nyordanov
Copy link
Contributor Author

In this case shouldn't if ( true ): throw an error?

@JDGrimes
Copy link
Contributor

JDGrimes commented Dec 1, 2014

Yes, in my opinion. I always use the space there as well.

@nyordanov
Copy link
Contributor Author

OK, it can be changed easily, but let's hear from @westonruter first?

@westonruter
Copy link
Member

There are 44 instances of else : in Core, and 13 instances of else:. In terms of raw numbers, it would seem else : wins, but since both exist in core then it's probably better to not mandate one or the other.

@nyordanov could you add a new property to the sniff which would control the behavior for whether a space is required or not?

…re_colon_check is actually WordPress_Sniffs_WhiteSpace_ControlStructureSpacingSniff::$space_before_colon_required
@nyordanov
Copy link
Contributor Author

I've added public $space_before_colon_required = true; in ac62ce6

When the property is set to false, it's required that there is no space before T_COLON. The default value requires that there is a space.

@GaryJones
Copy link
Member

In this case shouldn't if ( true ): throw an error?

The last snippet in the https://make.wordpress.org/core/handbook/coding-standards/php/#brace-style section has this documented with a space. I'd be happy to see : treated like a { and require the addition of a space before it as part of the definitive standard - anything else would be inconsistent.

@nyordanov
Copy link
Contributor Author

@GaryJones it does now, see the tests in 5afd52c

The default settings of the sniff require that there is T_WHITESPACE before T_COLON when the T_COLON is a scope_opener of a control structure.

@GaryJones
Copy link
Member

@GaryJones it does now, see the tests in 5afd52c

👍 Good job :-)

@westonruter
Copy link
Member

@nyordanov This is some quality code. 👍 One thing, however, is the naming of the space_before_colon_required property. Personally if I were to set this to false then I would expect it a space to be optional. But it seems actually that if you set it false than an error will be raised if there is a space. So perhaps this property should be changed from a simple boolean to a string:

'space_before_colon' => 'required' # default
'space_before_colon' => 'optional' # same as if suppressing NoSpaceBetweenStructureColon
'space_before_colon' => 'forbidden' # your preferred style

@JDGrimes
Copy link
Contributor

Related #17.

westonruter added a commit that referenced this pull request Jan 2, 2015
Fix T_ELSE immediately followed by T_COLON
@westonruter westonruter merged commit 13e2142 into WordPress:develop Jan 2, 2015
@GaryJones GaryJones added this to the 0.4.0 milestone Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants