Skip to content

Added yield in Generic.LowerCaseKeyword#1458

Closed
gmponos wants to merge 4 commits intosquizlabs:masterfrom
gmponos:add_yield_in_lowerCaseKeywordSniff
Closed

Added yield in Generic.LowerCaseKeyword#1458
gmponos wants to merge 4 commits intosquizlabs:masterfrom
gmponos:add_yield_in_lowerCaseKeywordSniff

Conversation

@gmponos
Copy link
Copy Markdown
Contributor

@gmponos gmponos commented May 9, 2017

No description provided.

@gmponos
Copy link
Copy Markdown
Contributor Author

gmponos commented May 9, 2017

This is weird. The current travis failed for all PHP versions except from 7. I have 7 and it seems that's why it passed also locally.

@gmponos gmponos force-pushed the add_yield_in_lowerCaseKeywordSniff branch from 8b54c5a to f083e70 Compare May 9, 2017 17:58
@gmponos
Copy link
Copy Markdown
Contributor Author

gmponos commented May 9, 2017

Any help on how I can make the travis green? After the new commit it's failing only for 5.4 version but not sure how to fix it.

$error = 'PHP keywords must be lowercase; expected "%s" but found "%s"';
$data = array(
strtolower($keyword),
$expected,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for this change I could not hold myself 😛

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You may also want to change this on line 128 below in that case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.. nice catch...

@jrfnl
Copy link
Copy Markdown
Contributor

jrfnl commented May 9, 2017

Any help on how I can make the travis green?

I suspect that PHPCS does not backfill for the yield keyword which was only introduced in PHP 5.5, so in PHP 5.4 it would be a T_STRING with the content yield you'd need to look for.

[Edit] As a side-note: this should probably be fixed (backfilled) in the Tokenizer, rather than fixed in the sniff itself. Preferably for both the 2.x as well as the 3.x branches.

function (array $a) {}
const PRIVATE;
HttpStatus::CONTINUE;
function
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This removes a very specific test case that is checking for an error that used to occur when the file ends with function (as happens when using check-as-you-type plugins). The commit is here: 88d3f29

When you see syntax errors in tests files, they are almost always there for a specific reason, so it's best to check the test file history just to be sure it's not a mistake.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gmponos
Copy link
Copy Markdown
Contributor Author

gmponos commented Jul 23, 2017

:( Closing since this #1492 covers more cases.

@gmponos gmponos closed this Jul 23, 2017
@gmponos gmponos deleted the add_yield_in_lowerCaseKeywordSniff branch July 25, 2017 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants