Exception to the newline requirement on the first member var after class opener. #1355
Conversation
wilsonge
force-pushed
the
first_class_new_line
branch
2 times, most recently
from
d45f4b6
to
1e06b8a
Compare
|
@gsherwood any thoughts on merging this? |
| $error = 'Expected %s blank lines before first member var; %s found'; | ||
| } | ||
|
|
||
| if ($foundLines === $this->spacing || ($isFirst === true && $foundLines === $this->firstMemberSpacing)) { |
There was a problem hiding this comment.
I found a bug with this line...
If I have a case where $this->spacing is set to 1 and $this->firstMemberSpacing is set to 0 and $isFirst is true, then if I happen to have exactly one space above the first member, I would expect to receive an error saying:
Expected 0 blank lines before first member var; 1 found
However, since $foundLines === $this->spacing is evaluated first, and it is true (since $foundLines is 1), then no error occurs.
I think the proper approach would be to change this line to:
if ($foundLines === $expected) {
|
As many popular projects (some references in #1464) use no spacing between the class opening and first var declaration, shouldn't |
It must be defaulted to 1 to maintain backwards compatibility. Projects that require no spaces are probably not even using the current sniff as it is not configurable, so there is no BC issue with them. |
…fter class opener. Based on the draft PHPCS2 Joomla Custom member var sniff https://github.com/photodude/coding-standards/blob/phpcs-2/Joomla/Sniffs/WhiteSpace/MemberVarSpacingSniff.php#L102-L137
(final round?)
wilsonge
force-pushed
the
first_class_new_line
branch
from
1414dc6
to
f3c1607
Compare
|
Rebased to fix conflicts |
|
@wilsonge would you fix the CS items FILE: src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php
23 | ERROR | [x] Expected "integer" but found "int" for @var tag in member variable comment
31 | ERROR | [x] Expected "integer" but found "int" for @var tag in member variable comment |
|
Done :) Thanks for the ping :) |
|
You are welcome, thanks for fixing those items. |
| $error = 'Expected %s blank lines before first member var; %s found'; | ||
| } | ||
|
|
||
| if ($foundLines === $this->spacing || ($isFirst === true && $foundLines === $this->firstMemberSpacing)) { |
There was a problem hiding this comment.
I got an error when I pass in zero as a parameter:
<property name="firstMemberSpacing" value="0" />It seems to me a cast to int is missing:
$foundLines === (int)$this->firstMemberSpacing
|
Thanks for this PR, although I didn't merge it in as I did things a little differently once I added a bunch of test cases. See the linked commit for the diff if you are interested. |
|
thanks @gsherwood glad to see this feature added. |
|
Thanks! |
Based on the draft PHPCS2 Joomla Custom member var sniff https://github.com/photodude/coding-standards/blob/phpcs-2/Joomla/Sniffs/WhiteSpace/MemberVarSpacingSniff.php#L102-L137
Addresses issues #920 and #725
This is a successor PR to #972 based on the feedback by @gsherwood which @photodude hadn't had time to work on yet