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] Do not build SnakList on top of ArrayObject #763

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiemowmde
Copy link
Contributor

The main reason to touch this file again is the awkward complexity it had before. Note that the data structure was build in a way that it kept track of two keys for each element. One key is the snak hash, the other one whatever you provided via the constructor.

WIP mostly because of the Iterator methods (rewind, current, and so on). These expose the internal state of the $this->snaks array. I'm not sure if this is robust enough. The array pointer might change when using other SnakList methods.

@JeroenDeDauw
Copy link
Contributor

The index change is nice though technically a breaking change. Since making a major release is currently very expensive, it might be better to avoid this one. We could also just present it is not a breaking change... though that is a bit scary. Something could be relying on the old behavior.

As to the Iterator stuff: you can implement IteratorAggregate instead. And in its getIterator method, just do

yield from $this->snaks;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants