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 google.api.core.helpers.general_helpers.wraps #4166

Merged
merged 1 commit into from
Oct 12, 2017

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.

@theacodes theacodes mentioned this pull request Oct 16, 2017
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