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 logging client #1422

Merged
merged 1 commit into from
Jan 29, 2016
Merged

Add logging client #1422

merged 1 commit into from
Jan 29, 2016

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

# See the License for the specific language governing permissions and
# limitations under the License.

# pylint: disable=missing-docstring

This comment was marked as spam.

This comment was marked as spam.

del self.stub

# Page descriptors
_LIST_LOG_METRICS_DESCRIPTOR = page_descriptor.PageDescriptor(

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.

"""
Lists log entries. Use this method to retrieve log entries from Cloud
Logging. For ways to export log entries, see
[Exporting Logs](/logging/docs/export).

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