Skip to content

Conversation

@alixhami
Copy link
Contributor

@alixhami alixhami commented Dec 4, 2018

This PR fixes the extract table sample to properly set the file format. Previously, the format was being improperly set, which resulted in defaulting to 'CSV' every time. This was identified by an internal bug report. The sample now also follows the python canonical sample more closely, though the PHP library cannot yet extract a public table.

@alixhami alixhami added the bug label Dec 4, 2018
@alixhami alixhami requested a review from bshaffer December 4, 2018 18:30
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

This looks good, one minor change, but why is it we don't have to wait for the job to complete anymore? Is that because the runJob method waits for completion? Is there an asyncRunJob?

@alixhami
Copy link
Contributor Author

alixhami commented Dec 4, 2018

I'm not sure why all that logic was in there to poll the job; maybe a library update? The current reference docs for runJob specify that it waits for the job to complete.

Edit: you can run async with startJob but we tend to avoid that for the sake of simplicity in samples.

@bshaffer
Copy link
Contributor

bshaffer commented Dec 4, 2018

Cool, then a side-note is we should remove all the job logic on the other samples, except maybe one to illustrate how to use startJob

@alixhami
Copy link
Contributor Author

alixhami commented Dec 4, 2018

I'll update them to follow canonical and the sample rubric as they require updates. If I have time, though, I might do a big update again. The async job stuff should probably have its own page in the docs if it's going to be used in a sample. I wouldn't want to arbitrarily set the copy job sample, for example, to be the async sample because I'd be worried (a) people would assume async is necessary for copy jobs specifically or (b) they won't know to look for the copy job sample when they want to do async job processing.

@bshaffer bshaffer merged commit 0ea346e into master Dec 4, 2018
@bshaffer bshaffer deleted the bq-extract-bug branch December 4, 2018 19:05
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.

2 participants