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.

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

[WIP] feature/countable stream iterator #346

Merged
merged 10 commits into from
Sep 23, 2018

Conversation

basz
Copy link
Member

@basz basz commented Sep 14, 2018

@prolic This is what I came up with. As always naming is an issue, php isn't consistent when it comes to interface naming. IMO 'Iterator' should have been called 'Iterable'.

This PR;

  • introduces an StreamIterator namespace.
  • provides an abstract StreamIterator class. It implements Iterator and Countable.
  • implements an EmptyStreamIterator and an ArrayStreamIterator from the abstract.
  • changes the EventStore implementations load and loadReverse methods to return a StreamIterator implementation. It does this without changing the return type of Iterator. It does not seem not possible to change the Prooph\EventStore\ReadOnlyEventStore interface to have a return type of StreamIterator without a BC break. :-( I do believe it is a minor BC as 'probably' will only break tests that use ArrayIterator or EmptyIterator (like you did).

Q: Is there a reason you are utilizing the EmptyIterator over an empty ArrayIterator?

Although I also provided an EmptyStreamIterator (you were using EmptyIterator) I think an ArrayStreamIterator (with an empty array) would be just as good. Both are countable now, but it might be a possibly BC break. Probably anyone currently checking for $i instanceof EmptyIterator somewhere.

@coveralls
Copy link

coveralls commented Sep 14, 2018

Coverage Status

Coverage increased (+0.0007%) to 99.007% when pulling c23e10d on basz:origin/feature/countable-stream-iterator into a6389e3 on prooph:master.

Copy link
Member

@prolic prolic left a comment

Choose a reason for hiding this comment

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

looks good.

use Countable;
use Iterator;

abstract class StreamIterator implements Countable, Iterator
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be an interface extending the other two, as there is not any implementation here.

@prolic
Copy link
Member

prolic commented Sep 14, 2018

The iterator instance returned is an implementation detail. We guarantee only the iterator interface as return type, therefore this is not a bc.

@prolic prolic self-assigned this Sep 14, 2018
@basz basz force-pushed the origin/feature/countable-stream-iterator branch from d48c946 to c23e10d Compare September 23, 2018 09:29
@prolic prolic merged commit ac8d2be into prooph:master Sep 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants