Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Integrate _OperationFuture into Python LRO #891

Merged
merged 7 commits into from
Jan 10, 2017
Merged

Conversation

evaogbe
Copy link
Contributor

@evaogbe evaogbe commented Jan 3, 2017

Implements LRO for Python.

Depends on googleapis/gax-python#142 to generate functioning code.

@codecov-io
Copy link

codecov-io commented Jan 4, 2017

Current coverage is 82.87% (diff: 82.35%)

Merging #891 into master will decrease coverage by 0.01%

@@             master       #891   diff @@
==========================================
  Files           334        334          
  Lines         12979      13001    +22   
  Methods           0          0          
  Messages          0          0          
  Branches       1706       1712     +6   
==========================================
+ Hits          10759      10775    +16   
- Misses         1784       1789     +5   
- Partials        436        437     +1   

Powered by Codecov. Last update 79f9ed7...0d02b53

@evaogbe evaogbe changed the title Integration _OperationFuture into Python LRO Integrate _OperationFuture into Python LRO Jan 4, 2017
Copy link
Contributor

@landrito landrito left a comment

Choose a reason for hiding this comment

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

You should also add some usage information to the example of methods returning an OperationFuture.

@evaogbe
Copy link
Contributor Author

evaogbe commented Jan 5, 2017

@landrito Something like this?

          >>> from google.cloud.gapic.example.library.v1 import library_service_client
          >>> api = library_service_client.LibraryServiceClient()
          >>> name = api.book_path('[SHELF_ID]', '[BOOK_ID]')
          >>> response = api.get_big_book(name)
          >>> response.add_done_callback(do_something_with)

@landrito
Copy link
Contributor

landrito commented Jan 5, 2017

A bit more verbose. This is what the example is for Ruby.

For python I'd imagine it to look like.

from google.cloud.gapic.example.library.v1 import library_service_client
api = library_service_client.LibraryServiceClient()
name = api.book_path('[SHELF_ID]', '[BOOK_ID]')
operation_future = api.get_big_book(name)

def callback(operation_future):
    # Handle response.
    result = operation_future.result


operation_future.add_done_callback(callback)

# Handle metadata.
metadata = operation_future.metadata

edit: fix error.

@@ -233,6 +233,17 @@ public String addImportLocal(String moduleName, String attributeName) {
return addImport(ImportType.APP, moduleName, attributeName).shortName();
}

/** Add an import for the proto associated with the given type. */
private PythonImport addImportForType(TypeRef type) {

This comment was marked as spam.

Copy link
Contributor

@lukesneeringer lukesneeringer left a comment

Choose a reason for hiding this comment

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

Other than the need for documentation, this looks fine to me. Ernest's more verbose version is slightly erroneous (it should read operation_future.add_done_callback(callback), and should probably use a different variable name for the callback argument itself) but seems fine in principle. I also think explanatory comments on documentation like that is really useful, even if they seem like overkill to us.

@evaogbe
Copy link
Contributor Author

evaogbe commented Jan 5, 2017

PTAL

Copy link
Contributor

@geigerj geigerj left a comment

Choose a reason for hiding this comment

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

LGTM, but please wait to merge until the GAX change is submitted.

@evaogbe evaogbe merged commit 34deb54 into googleapis:master Jan 10, 2017
michaelbausor pushed a commit to michaelbausor/toolkit that referenced this pull request Jan 12, 2017
* Integrate _OperationFuture into Python LRO

* Add LRO specific usage for method sample

* Change addImportForType to addImportForMessage.

Fixes potential NPE if called with a type that is not a MessageType.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants