Corrected the exception class used and removed unnecessary strtolower in MethodScanner::setVisibility #140
Conversation
|
@fhein patch looks fine, but a test case verifying the expected thrown exception type is needed. See https://github.com/zendframework/zend-code/blob/6b1059db5b368db769e4392c6cb6cc139e56640d/test/Scanner/MethodScannerTest.php You can push to your current branch to update this PR 👍 |
- Added test case for InvalidArgumentException - Added test case for switch
|
Done. Plus a bit. :) Anything left to do? |
test/Scanner/MethodScannerTest.php
Outdated
| { | ||
| $methodScanner = new MethodScanner([]); | ||
|
|
||
| self::assertTrue($methodScanner->setVisibility(T_PUBLIC) === $methodScanner); |
test/Scanner/MethodScannerTest.php
Outdated
| $methodScanner = new MethodScanner([]); | ||
|
|
||
| self::assertTrue($methodScanner->setVisibility(T_PUBLIC) === $methodScanner); | ||
| self::assertTrue($methodScanner->setVisibility(T_PROTECTED) === $methodScanner); |
test/Scanner/MethodScannerTest.php
Outdated
|
|
||
| self::assertTrue($methodScanner->setVisibility(T_PUBLIC) === $methodScanner); | ||
| self::assertTrue($methodScanner->setVisibility(T_PROTECTED) === $methodScanner); | ||
| self::assertTrue($methodScanner->setVisibility(T_PRIVATE) === $methodScanner); |
test/Scanner/MethodScannerTest.php
Outdated
| @@ -110,4 +111,27 @@ public function testMethodScannerWorksWithSingleAbstractFunction() | |||
|
|
|||
| self::assertTrue($method->isAbstract()); | |||
| } | |||
|
|
|||
| /** | |||
| * @expectedException \Zend\Code\Exception\InvalidArgumentException | |||
There was a problem hiding this comment.
Please use $this->expectException() instead, and right before the call that causes the exception to be thrown.
test/Scanner/MethodScannerTest.php
Outdated
| $methodScanner = new MethodScanner([]); | ||
|
|
||
| $invalidArgument = 42; | ||
| self::assertTrue(! in_array($invalidArgument, [T_PUBLIC, T_PROTECTED, T_PRIVATE])); |
There was a problem hiding this comment.
Remove this assertion - if the code stops throwing exceptions, we know we broke BC :-)
There was a problem hiding this comment.
I suggest to stay with this one. Ok, it's not likely that the constant values of T_PUBLIC et al change. ;) But if the values were changed, the change could break the test (e.g. T_PUBLIC = 42).
BC would not be affected (in the particular context of this method).
Ok?
There was a problem hiding this comment.
You are testing something unrelated here, that's why I asked for removal.
There was a problem hiding this comment.
I see. Now I make sure that the argument is invalid without asserting that.
|
Looking good! |
|
:) . This was very helpful to setup and test my environment. Thanks. After a short review of Roave/BetterReflection I think it is better to stop fiddling around with the scanner stuff here. I've got some spare time I could spent helping out. If you like and if there's demand, feel free to give me a try and direct me to somewhere where help is needed. |
…ted_Exception_class_used Corrected the exception class used and removed unnecessary strtolower in MethodScanner::setVisibility
|
Thanks, @fhein! |
Exception - which is used as exception class within setVisibility - is not a class but a (sub)namespace. InvalidArgumentException likely is the intended exception class here.
This is my first submission. Wanted to get my feet wet with the submission workflow. :)