PSR2/UseDeclaration: simplify the fix #1
Conversation
jrfnl
force-pushed
the
feature/test-is-bs-simplification
branch
2 times, most recently
from
99a42f5
to
e35e559
Compare
| // Find either the start of the next line or the beginning of the next statement, | ||
| // whichever comes first. | ||
| for ($end = ++$end; $end < $phpcsFile->numTokens; $end++) { | ||
| if (isset(Tokens::$emptyTokens[$tokens[$end]['code']]) === false) { |
There was a problem hiding this comment.
It seems like some of the tokens in Tokens::$emptyTokens shouldn't be allowed: I think that only inline comments should be allowed and multi-line comments shouldn't be: https://github.com/dhensby/PHP_CodeSniffer/blob/master/src/Util/Tokens.php#L435
I don't think stuff like this should pass...
use Foo\Bar; /*
* This multi-line comment shouldn't be allowed here.
*/
So, this sniff should build and use an array of $inlineCommentTokens.
There was a problem hiding this comment.
I don't think stuff like this should pass...
Have you tested it ? Not sure about previously, but this will definitely throw an error with the code I submitted.
Though I have to admit, I think the fixer will fail on it. I'll add an additional unit test for that and have a go at getting the fixer sorted.
So, this sniff should build and use an array of $inlineCommentTokens.
@jrfnl @dhensby Do you agree?
No. Multi-line block comments are tokenized as T_COMMENT as well, so that wouldn't solve anything.
There was a problem hiding this comment.
@jrfnl I'll admit I didn't test it. Sorry! I hadn't realized there isn't a separate token for inline comments.
jrfnl
force-pushed
the
feature/test-is-bs-simplification
branch
from
e35e559
to
fd8050f
Compare
|
@iammattcoleman @dhensby I've updated the PR with yet more unit tests and an additional fix to allow the fixer to function correctly for:
|
... covering different variations of comments after the last use statement.
…us combinations of comments
jrfnl
force-pushed
the
feature/test-is-bs-simplification
branch
from
fd8050f
to
1530543
Compare
DisallowAlternativePHPTags: remove pre-PHP 5.4 work-around
Based on a review of upstream PR squizlabs#1893, please find this as an optional suggestion for simplifying the logic a little.
All unit tests - including the new ones - still pass with this change included.
Added one more unit test case which looked like it wasn't covered yet.