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

Added resumptionToken argument to allow imports to continue a previous import. #36

Merged

Conversation

seanBlommaert
Copy link
Contributor

Hi,

Thanks for your awesome work. While working on a very large import it seemed nice to be able to allow batches of records in stead of importing everything at once. The applications keeps tracks of the resumptionToken and stores it locally. When doing the next batch, we start at the last known resumptionToken.

For this to work I've added the resumptionToken as an argument for ListRecords and ListIdentifiers (and off course to the RecordIterator). I've also added public methods to fetch the resumptionToken and the current batch, since we need the batch to determine when we want to store the new resumptionToken.

Would you consider adding this?

By the way, I've used this library to create a migration provider for Drupal (https://www.drupal.org/project/migrate_oaipmh). The Drupal migrate module allows for batch imports, so that was the main reason to start this. But I think it would make sense in other cases as well.

@seanBlommaert seanBlommaert mentioned this pull request Jul 8, 2016
@caseyamcl
Copy link
Owner

Hi There,

Sorry for the delay! I've been out and about doing other things.

If you're still interested in merging this pull request, would you mind writing some Unit Tests for the change? I think it would work quite well.

@seanBlommaert
Copy link
Contributor Author

I've extended the existing tests to include the new parameter. Is this enough? There is no test for the recordIterator, but I guess this is a seperate issue.

@caseyamcl
Copy link
Owner

This looks good! Thanks for the dilligence. Will release a new version today.

@caseyamcl caseyamcl merged commit 1f9a8d5 into caseyamcl:master Jul 22, 2016
@seanBlommaert
Copy link
Contributor Author

No problem! Thnx for the merge :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants