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

Squiz.WhiteSpace.SuperfluousWhiteSpace ignoreBlankLines property ignores whitespace after single line comments #1828

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Jan 4, 2018

Setting ignoreBlankLines to true, would also cause the sniff to ignore single-line comments with superfluous whitespace at the end.

Includes unit test.

@@ -192,6 +192,7 @@ public function process(File $phpcsFile, $stackPtr)

// Ignore blank lines if required.
if ($this->ignoreBlankLines === true
&& trim($tokens[$stackPtr]['content']) === ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be easier to just use && $tokens[$stackPtr]['code'] === T_WHITESPACE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. (Commit amended)

@gsherwood gsherwood added this to the 3.2.3 milestone Jan 4, 2018
…gnore blank lines

Setting `ignoreBlankLines` to true, would now also cause the sniff to ignore single-line comments with superfluous whitespace at the end.

Includes unit test
@jrfnl jrfnl force-pushed the feature/bugfix-superfluous-whitespace-ignore-blank-lines branch from 8ac478d to 5e70aeb Compare January 4, 2018 11:46
@gsherwood gsherwood changed the title Squiz/SuperfluousWhiteSpace: bug fix - ignoreBlankLines should only ignore blank lines Squiz.WhiteSpace.SuperfluousWhiteSpace ignoreBlankLines property ignores whitespace after single line comments Jan 4, 2018
@gsherwood gsherwood merged commit 5e70aeb into squizlabs:master Jan 4, 2018
gsherwood added a commit that referenced this pull request Jan 4, 2018
@gsherwood
Copy link
Member

Thanks for that change

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.

2 participants