Add missing return statement. #7346
Conversation
|
|
|
Please supply a unit test so that this does not happen in the future. |
|
Um… it's only missing if you expect a fluent interface. Unfortunately, it's documented that way, so I can see why the expectation is there. :) As @mwillbanks notes, please add a unit test so that we know the fluent contract exists and is expected. |
|
This is the first time I am dealing with unit tests, so bear with me :) |
|
@ffraenz You could use assertSame instead of assertEquals, so it will assert if the return is the same instance and not another instance of the same type. Also i didn't see the point of asserting setFieldType in the same test. You could at least split it and make the test name more explicit, like ReturnsSameRendererInstanceWhenResolverIsSet and ReturnsSameRendererInstanceWhenFieldTypeIsSet. |
|
@fabiocarneiro That makes perfect sense to me, thank's! |
|
@weierophinney may be it will be better NOT to introduce fluent interface? It is kinda bad and that is not BC break since it never was there. |
|
@Xerkus I agree with you, but some ppl consider docblocks change as bc break... |
|
|
||
| public function testReturnsSameRendererInstanceWhenResolverIsSet() | ||
| { | ||
| $returnValue = $this->renderer->setResolver(null); |
There was a problem hiding this comment.
You will need to actually set a resolver here.
|
@Xerkus I'd agree with you, except that this setter would then be inconsistent with the rest of the class, which does implement a fluent interface on existing setters. As such, I'd consider this a necessary fix. |
Add missing return statement.
Add missing return statement.
No description provided.