-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Fixed typos in the expectedException annotations #19243
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
Conversation
Not checked it fully, but i found -+ 34 violations. I.e. i guess there are more occurrences for this specific problem. Also the root namespace is still missing now.. this annoys editors ;-) @fabpot can you confirm these are false positives? Makes sense to test the namespaced exception is thrown... rather than some potential subclass. |
@@ -57,7 +55,7 @@ public function testGetUrl() | |||
} | |||
|
|||
/** | |||
* @expectedException LogicException | |||
* @expectedException Symfony\Component\Asset\Exception\LogicException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should start with \ (unfortunately)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPUnit doesn't resolve them like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what we are using everywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add them if you want, but phpunit will just trim them.
The FQCN is defined without a leading slash. The leading slash mearly indicates to escape to the global namespace. If your IDE doesn't understand that PHPUnit doesn't resolve thing in this way, then it's broken. |
Understand.. im fine with both. But lets be consistent :) Still.. what if PHPunit fixes this, how should it be then? Cause potentially there can be a class |
PHPUnit does do anything relative to a namespace. The fact that might be true is irrelevant. |
Like i said my only concern is it requires a lot of refactoring whenever PHPunit implements this. Assuming its a valid usecase.. however i guess its not gonna happen because of this. |
I don't think PHPUnit will ever implement this. |
@ro0NL check this out: sebastianbergmann/phpunit#1633 |
Sorry.. i cant really play with it right now, but to clearify.. this works different then as opposed to To sum up..
For both i would like to see a fixer optimizing this to some consistent variant. edit: @keradus 👍 |
Thank you @GrahamCampbell. |
…Campbell) This PR was squashed before being merged into the 2.7 branch (closes #19243). Discussion ---------- Fixed typos in the expectedException annotations | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A PHPUnit ignores any imports when resolving these. You must always reference the FQCN. Commits ------- b36de36 Fixed typos in the expectedException annotations
PHPUnit ignores any imports when resolving these. You must always reference the FQCN.