Skip to content

Conversation

@theacodes
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 12, 2017
@theacodes
Copy link
Contributor Author

@dhermes As an aside, how would you feel if I dropped the google.api.core.helpers package and just moved all the modules up one, e.g., google.api.core.general_helpers, google.api.core.grpc_helpers, etc.?

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Oct 12, 2017

As an aside, how would you feel if I dropped the google.api.core.helpers package and just moved all the modules up one, e.g., google.api.core.general_helpers, google.api.core.grpc_helpers, etc.?

-0, but they arguably do not need to all end in _helpers. What about google.api.code.helpers.general? And the usage pattern could be...

from google.api.core import helpers

@helpers.general.wraps(thing)
def other_thing

My concern is that there will be a lot of helpers things interspersed with other stuff. And while helpers.general looks kind of dumb, helpers.grpc makes total sense.

The general helpers could also just go into __init__.py. helpers.wraps is fine.

@dhermes
Copy link
Contributor

dhermes commented Oct 12, 2017

@jonparrott I am big time 👍 on reducing the number of nested packages.

Copy link
Contributor

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

LGTM, though I'd like to see a code snippet with the failure you intend to workaround here.

assert replacement() == 42


def test_wraps_partial():

This comment was marked as spam.

This comment was marked as spam.

@theacodes
Copy link
Contributor Author

@jonparrott I am big time 👍 on reducing the number of nested packages.

I'll send a separate PR to do that.

@theacodes theacodes merged commit 99c4498 into googleapis:master Oct 12, 2017
@theacodes theacodes deleted the wraps-makes-me-sad branch October 12, 2017 17:11
@jba
Copy link
Contributor

jba commented Oct 12, 2017

Can someone merge this into the bigquery-b2 branch? I'm not sure how to do that.

@dhermes
Copy link
Contributor

dhermes commented Oct 12, 2017

@jba I can do it. Will a rebase be OK?

@jba
Copy link
Contributor

jba commented Oct 12, 2017

@tswast What's the right way to get this into bigquery-b2? Maybe just cherry-pick?

@tswast
Copy link
Contributor

tswast commented Oct 12, 2017

Probably best to rebase bigquery-b2 on master rather than cherry-pick. We'll have to rebase for the final push anyway.

@dhermes
Copy link
Contributor

dhermes commented Oct 12, 2017

@tswast I agree. Do you need me to do that or you guys have it covered?

@tswast
Copy link
Contributor

tswast commented Oct 12, 2017

@dhermes @jba Rebase done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants