FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add google.api.core.helpers.general_helpers.wraps by theacodes · Pull Request #4166 · googleapis/google-cloud-python · GitHub

Add google.api.core.helpers.general_helpers.wraps - #4166

Merged
theacodes merged 1 commit into
googleapis:masterfrom
theacodes:wraps-makes-me-sad
Oct 12, 2017
Merged

Add google.api.core.helpers.general_helpers.wraps#4166
theacodes merged 1 commit into
googleapis:masterfrom
theacodes:wraps-makes-me-sad

Conversation

Copy link
Copy Markdown
Contributor

No description provided.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 12, 2017

Copy link
Copy Markdown
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 commented Oct 12, 2017
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

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

dhermes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor Author

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

I'll send a separate PR to do that.

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

jba commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

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

dhermes commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

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

jba commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

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

tswast commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

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

dhermes commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

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

tswast commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

@dhermes @jba Rebase done.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL