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

[Data Liberation] Introduce WP_Entity_Reader_Iterator #2122

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

adamziel
Copy link
Collaborator

Introduces a new WP_Entity_Reader_Iterator( $entity_reader ) class to separate the iterator interface from the entity reader interface.

Rationale

Mixing the entity reader interface with the iterator interface is impractical, error-prone, and confusing:

  • Iterator methods overlap with the EntityReader methods and it's confusing to have both next() and next_entity() with different semantice.
  • The iterator interface expects the data to be ready on the first current() call, but the EntityReader interface assumes the first get_current_entity() call returns null until next_entity() is called
  • Maintaining a copy&paste current(), next() etc. methods over a number of classes is a maintenance burden

Testing instructions

TBD. I haven't tested this PR yet.

There are no server-side unit tests for WP_Stream_Importer in trunk yet. AFAIR @zaerl you had something like that in your branch? It could come handy for testing this PR.

Aside of that, manual testing in the Data Liberation plugin is appropriate. Instructions TBD.

Introduces a `new WP_Entity_Reader_Iterator( $entity_reader )` class to
separate the iterator interface from the entity reader interface.

 ## Rationale

Mixing the entity reader interface with the iterator interface is impractical, error-prone, and
confusing:

* Iterator methods overlap with the EntityReader methods and it's
  confusing to have both `next()` and `next_entity()` with different
  semantice.
* The iterator interface expects the data to be ready on the
  first current() call, but the EntityReader interface assumes the first
  `get_current_entity()` call returns null until `next_entity()` is
  called
* Maintaining a copy&paste `current()`, `next()` etc. methods over a
  number of classes is a maintenance burden

 ## Testing instructions

TBD. I haven't tested this PR yet.

There are no server-side unit tests for WP_Stream_Importer in
`trunk` yet. AFAIR @zaerl you had something like that in your branch? It
could come handy for testing this PR.

Aside of that, manual testing in the Data Liberation plugin is
appropriate. Instructions TBD.
@zaerl
Copy link
Collaborator

zaerl commented Jan 10, 2025

I appreciated this approach here, good work. As already discussed, this is the best approach, and I think it will help with a trick I needed to solve in my PR.

AFAIR @zaerl you had something like that in your branch? It could come handy for testing this PR.

Let me try my new tests and see if they work without being touched in this context. If they do, we can merge this, and I will add them later with my PR.

@zaerl zaerl self-requested a review January 10, 2025 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants