Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Typehint against DomainEventStreamInterface instead of concrete implementation #71

Merged
merged 1 commit into from
Nov 6, 2014
Merged

Typehint against DomainEventStreamInterface instead of concrete implementation #71

merged 1 commit into from
Nov 6, 2014

Conversation

rjkip
Copy link
Contributor

@rjkip rjkip commented Oct 31, 2014

No description provided.

wjzijderveld
wjzijderveld reviewed Nov 3, 2014
View reviewed changes
src/Broadway/EventSourcing/EventStreamDecoratorInterface.php
* @param string $aggregateType
* @param string $aggregateIdentifier
* @param DomainEventStream $eventStream
* @param string $aggregateType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align the variables with each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@wjzijderveld
Copy link
Member

We probably should fix this as well sometime, but that's not for this pull 😃: https://github.com/rjkip/broadway/blob/499fcecfbee58062b523a146e058fe54405cfd50/src/Broadway/EventSourcing/MetadataEnrichment/MetadataEnrichingEventStreamDecorator.php#L60

@rjkip
Copy link
Contributor Author

rjkip commented Nov 3, 2014

We probably should fix this as well sometime, but that's not for this pull : https://github.com/rjkip/broadway/blob/499fcecfbee58062b523a146e058fe54405cfd50/src/Broadway/EventSourcing/MetadataEnrichment/MetadataEnrichingEventStreamDecorator.php#L60

Not seeing it, what's there to fix? :o

@wjzijderveld
Copy link
Member

As input we now typehint the interface, but the output is always an instance of DomainEventStream.

@wjzijderveld
Copy link
Member

👍

@wjzijderveld
Copy link
Member

ping @qandidate-labs/broadway

@asm89
Copy link
Member

asm89 commented Nov 6, 2014

@wjzijderveld Can you create an issue for the other thing? (not sure if it's actually a problem though :) )

As for this pull 👍 .

wjzijderveld added a commit that referenced this pull request Nov 6, 2014
@wjzijderveld
Merge pull request #71 from rjkip/maintenance/event-stream-typehint
6093c7c 
Typehint against DomainEventStreamInterface instead of concrete implementation
@wjzijderveld wjzijderveld merged commit 6093c7c into broadway:master Nov 6, 2014
@wjzijderveld wjzijderveld mentioned this pull request Nov 6, 2014
Closed
@rjkip rjkip deleted the maintenance/event-stream-typehint branch November 7, 2014 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rjkip @wjzijderveld @asm89