-
Notifications
You must be signed in to change notification settings - Fork 102
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
New line between @expectedException and @expectedExceptionMessage #101
Comments
…break between @ExpectedException and @expectedExceptionMessage. Opened issue: djoos/Symfony-coding-standard#101
you could consider aliasing your annotations? /**
* @Tests\expectedException \Exception
* @Tests\expectedExceptionMessage Don't do it.
*/
public function testThing() {} should be valid. |
I didn't think that worked with phpunit's annotation parser? |
i have no idea whether that's possible with phpunit's annotation parser tbh...
i don't think that's really the purpose of a coding standard and what you're basically asking is this coding standard to provide a way to break the rules based on sentiment. of course feel free to create a pull request for this and try to convince @djoos of it's pupose, but personally i'm not in favour. |
Hmmm, I must admit I wasn't originally in favor either, but that is a great point @CarsonF... |
@CarsonF it looks like all your references point to test classes which clearly do not comply to a lot of standards (no class comment, no function comments, multiple classes in one file, etc.). apart from that, if you have a look at the naming convention section of the coding standard, it's stated
so using the symfony source code as a reference to justify whether or not a sniff should be implemented might not be the way to go... even though my personal opinion is that test classes should comply to coding standards as well, it's always possible to ignore those in your ci configuration |
The
Commenting\AnnotationsSniff
currently doesn't like:But I don't think this looks right:
Could we add a blacklist for these? Possibly make it configurable?
The text was updated successfully, but these errors were encountered: