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

Fix error in VAN Target Export method. #704

Merged
merged 3 commits into from
Jul 8, 2022

Conversation

cmc333333
Copy link
Contributor

It looks like this method assumes VAN return a list of exported files
rather than a single target, but VAN currently only returns individual
targets: https://docs.ngpvan.com/reference/targetexportjobsexportjobid

They might've changed the API since this was last updated? In any event,
these tweaks return correct data in my recent testing!

It looks like this method assumes VAN return a *list* of exported files
rather than a single target, but VAN currently only returns individual
targets: https://docs.ngpvan.com/reference/targetexportjobsexportjobid

They might've changed the API since this was last updated? In any event,
these tweaks return correct data in my recent testing!
@cmc333333 cmc333333 changed the title Fix error VAN Target Export method. Fix error in VAN Target Export method. Jun 24, 2022
Copy link
Contributor

@bzupnick bzupnick 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 to me! I also like seeing the var names go from j and i to response. 👍

@cmc333333
Copy link
Contributor Author

Found another little optimization: rather than downloading the whole file into memory, we can let petl do its lazy loading thing.

@SorenSpicknall SorenSpicknall merged commit 496a320 into move-coop:main Jul 8, 2022
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.

3 participants