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

NativeConstantInvocationFixer - better constant detection #3823

Merged
merged 1 commit into from Jul 5, 2018

Conversation

gharlan
Copy link
Contributor

@gharlan gharlan commented Jun 9, 2018

Some previous failing test cases:

M_PI::foo();

function foo(M_PI $a) {}

function foo(): M_PI {}

$foo instanceof M_PI;

class x implements FOO, BAR, BAZ {}

use X\Y\{FOO, BAR as BAR2, BAZ};

class Foo { use Bar, M_PI { Bar::baz insteadof M_PI; } }

M_PI: goto M_PI; // ;)

@gharlan gharlan force-pushed the is-constant-invocation branch 4 times, most recently from 61d03f8 to 78e60bf Compare June 9, 2018 14:19
@SpacePossum
Copy link
Contributor

how about these as well than ;)

yield M_PI;
yield M_PI => M_PI;

@gharlan
Copy link
Contributor Author

gharlan commented Jun 9, 2018

@SpacePossum The check is done like this: "All T_STRING tokens are constant invocations, except those matching these conditions...".
So the challenge is to find all cases where a T_STRING is not a constant invocation.

But of course it is good to have also different test cases for the actual constant invocations, so I will add your test case! 👍

@Slamdunk
Copy link
Contributor

👍

@julienfalque julienfalque added this to the 2.12.2 milestone Jun 10, 2018
@julienfalque julienfalque added the RTM Ready To Merge label Jun 10, 2018
@Slamdunk
Copy link
Contributor

Shouldn't the new test cases also be added to the fixer's test, so refactoring TokenAnalyzer in the future will not mess up the specific fixer?

@julienfalque
Copy link
Member

Indeed, they should be added to the fixer's test too.

@julienfalque julienfalque removed the RTM Ready To Merge label Jun 11, 2018
@SpacePossum SpacePossum added the RTM Ready To Merge label Jun 22, 2018
@SpacePossum
Copy link
Contributor

Thanks @gharlan.

@SpacePossum SpacePossum merged commit a8a4ef2 into PHP-CS-Fixer:2.12 Jul 5, 2018
SpacePossum added a commit that referenced this pull request Jul 5, 2018
…gharlan, SpacePossum, keradus)

This PR was squashed before being merged into the 2.12 branch (closes #3823).

Discussion
----------

NativeConstantInvocationFixer - better constant detection

Some previous failing test cases:

```php
M_PI::foo();

function foo(M_PI $a) {}

function foo(): M_PI {}

$foo instanceof M_PI;

class x implements FOO, BAR, BAZ {}

use X\Y\{FOO, BAR as BAR2, BAZ};

class Foo { use Bar, M_PI { Bar::baz insteadof M_PI; } }

M_PI: goto M_PI; // ;)
```

Commits
-------

a8a4ef2 NativeConstantInvocationFixer - better constant detection
@SpacePossum SpacePossum removed the RTM Ready To Merge label Jul 5, 2018
@gharlan gharlan deleted the is-constant-invocation branch July 5, 2018 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants