outsourced configuration from ConfigurableAggregateTranslator - #238
Conversation
some more tests and docs have to be added
…performance added some more tests added doc blocks
prolic
left a comment
prolic
left a comment
There was a problem hiding this comment.
Generally I am okay with this change. If performance is a real issue for you (and I don't think this makes much of a difference) you can still implement the Translator-interface yourself.
|
|
||
| $instance = clone $this; | ||
| $instance->messageToEventCallback = $messageToEventCallback; | ||
| return $instance; |
|
|
||
| $instance = clone $this; | ||
| $instance->eventToMessageCallback = $eventToMessageCallback; | ||
| return $instance; |
|
|
||
| $instance = clone $this; | ||
| $instance->staticReconstituteFromHistoryMethodName = $staticReconstituteFromHistoryMethodName; | ||
| return $instance; |
|
|
||
| $instance = clone $this; | ||
| $instance->replayEventsMethodName = $replayEventsMethodName; | ||
| return $instance; |
|
|
||
| $instance = clone $this; | ||
| $instance->popRecordedEventsMethodName = $popRecordedEventsMethodName; | ||
| return $instance; |
| */ | ||
| public function it_returns_configured_message_to_event_callback() | ||
| { | ||
| $callback = function () {}; |
| $this->config->messageToEventCallback() | ||
| ); | ||
| } | ||
| } No newline at end of file |
| * @var AggregateTranslatorConfiguration | ||
| */ | ||
| private $configProphecy; | ||
|
|
|
|
||
| $historyEvents = new \ArrayIterator([$historyEvent->reveal()]); | ||
|
|
||
There was a problem hiding this comment.
remove @expectedException in phpdoc and use $this->expectException(...) instead.
|
Travis build failed, too. |
|
Allright, thanx for reviewing. I'll provide the fixes in the evening. |
fixed stuff reviewed by @prolic
|
@dropdevcoding Fun for another PR updating the copyright to 2017 instead of 2016? |
|
ah god damn it - this is a BC break because the constructor did change. @codeliner revert it? |
codeliner
left a comment
codeliner
left a comment
There was a problem hiding this comment.
@dropdevcoding I've added some comments and an idea how to avoid the BC break. Feel free to submit a second PR that we can merge without breaking BC. Please also add a note in the docs then submit a second PR
| $eventToMessageCallback = null, | ||
| $messageToEventCallback = null | ||
| ) { | ||
| $this->versionMethodName = $versionMethodName; |
There was a problem hiding this comment.
type assertions are missing. Best thing is to add a private set* method for each methodname and assert minLength their. Then you can use the setter in the constructor and in the with* methods
There was a problem hiding this comment.
Type assertions aren't missing, they are made within the dedicated with* methods for each property. I think this shouldn't be a problem since the constructor of configuration is private and only called within the factory method for default configuration.
There was a problem hiding this comment.
then you should make it private :P, currently I see public function __construct(
There was a problem hiding this comment.
constructor needs to stay public, otherwise BC again!
There was a problem hiding this comment.
hehe, not the one of AggregateTranslatorConfiguration because this is a new class ;)
| $staticReconstituteFromHistoryMethodName = null, | ||
| $eventToMessageCallback = null, | ||
| $messageToEventCallback = null) | ||
| public function __construct(AggregateTranslatorConfiguration $configuration = null) |
There was a problem hiding this comment.
BC break! to avoid this BC break we can do the following. Keep constructor as-is.
Add a static named constructor:
ConfigurableAggregateTranslator::fromConfiguration(AggregateTranslatorConfiguration $configuration)
There was a problem hiding this comment.
I'll propose some changes soon. Please have a look again then.
There was a problem hiding this comment.
Please have a look again. I admit it's kind of back and forth using the config without having a BC break but the intention is to have the config as single point of validation for itself (see #242).
Thanks for the hint @codeliner I changed the config's constructor to private.
3 participants
some more tests and docs have to be added