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

Add batch request callback hook #145

Merged
merged 4 commits into from
Feb 28, 2017
Merged

Conversation

jcoatgoogle
Copy link
Contributor

This allows custom error handling etc in the case of batch requests, similar to what is possible in single request mode.

Copy link
Contributor

@vilasj vilasj 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, would be good to add some tests for this.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.491% when pulling 8068315 on jcoatgoogle:jco/batch_callback into b71c414 on google:master.

@jcoatgoogle
Copy link
Contributor Author

@vilasj, I augmented a test to use this hook. I'm not sure if you want something more focused, or if it should be used more broadly, etc. I welcome any advice on how to best test this. Thanks!

@@ -185,7 +193,8 @@ def testSingleRequestInBatch(self):
'desired_request': desired_request,
})

api_request_responses = batch_api_request.Execute(FakeHttp())
api_request_responses = batch_api_request.Execute(
FakeHttp(), batch_request_callback=_Callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will pass if the callback is never executed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah drat, you're right. I've updated the test. Meant to add that originally...

@@ -150,9 +150,18 @@ def testRequestServiceUnavailable(self):
exceptions.HttpError)

def testSingleRequestInBatch(self):
desired_url = 'https://www.example.com'

callback_was_called = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Just curious, why list instead of just an integer count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python weirdness about callbacks and mutable variables. Calling a function on a variable will work 100% of the time, otherwise I need to mess with the global keyword.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for clarification.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 87.501% when pulling 33235a1 on jcoatgoogle:jco/batch_callback into b71c414 on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 87.501% when pulling 33235a1 on jcoatgoogle:jco/batch_callback into b71c414 on google:master.

@vilasj
Copy link
Contributor

vilasj commented Feb 28, 2017

Looks like there are some lint failures...

@jcoatgoogle
Copy link
Contributor Author

Yeah I saw the lint failures. I'll fix and resubmit.

@jcoatgoogle
Copy link
Contributor Author

@vilasj fixed the lint error.

@vilasj vilasj merged commit 1f57d98 into google:master Feb 28, 2017
@jcoatgoogle jcoatgoogle deleted the jco/batch_callback branch March 10, 2017 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants