Skip to content

Conversation

@saicheems
Copy link

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 27, 2016
@saicheems
Copy link
Author

@tbetbetbe

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Jan 27, 2016

General comment: Having things like

self.stub = api_utils.create_stub(
    logging_metrics_pb2.beta_create_MetricsServiceV2_stub,
    service_path,
    port,
    ssl_creds=ssl_creds,
    channel=channel,
    scopes=scopes)

just seems crazy to me. Why is it necessary to have a library to wrap a library? logging_metrics_pb2.beta_create_MetricsServiceV2_stub is supposed to be a useful object for making API calls. So why do we need to wrap it to make another useful object?

The stub also has generated methods exactly like the ones generated here. Why do we need generated methods to cover the already generated methods from gRPC?

a88


Aside: It also doesn't make it any easier to use ssl_creds (a pain point of gRPC).

@saicheems
Copy link
Author

Thanks for the feedback pass!
Will address everything in the morning

@tbetbetbe
Copy link

@anthmgoogle

@saicheems
Copy link
Author

Most general style nits addressed - the other issues we are tracking and intend to punt from this PR per offline discussion.
Mainly:

  1. Improved documentation (docstrings, argument types, ...)
  2. Collapsing the callable

The other quickly solvable issue I'll address in the next PR is making stub a property.

@dhermes
Copy link
Contributor

dhermes commented Jan 29, 2016

What is the goal for getting tests to pass here? We already have a fake gRPC for the Bigtable tests, so you might be able to get tests to pass by removing the dependencies that bring in grpcio.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@saicheems
Copy link
Author

Fixed setup.py ordering

dhermes added a commit that referenced this pull request Jan 29, 2016
@dhermes dhermes merged commit 984b73b into googleapis:grpc-logging-feature Jan 29, 2016
@dhermes dhermes mentioned this pull request Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants