Skip to content

Streaming: strip unused features #1209

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.

Sign up for GitHub

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

Jump to bottom
Merged
tseaver merged 17 commits into from
Nov 19, 2015
Merged

Streaming: strip unused features #1209

tseaver merged 17 commits into from
Nov 19, 2015

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Oct 29, 2015

Uses #1208 as a base.

  • Trim / normailze / document imports.
  • Rename functions / methods to match PEP 8.
  • Make constants / methods used outside their modules public.
  • Make functions / methods not imported outside their modules explicitly private.
  • Drop unused exceptions.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 29, 2015
@dhermes
Copy link
Contributor

dhermes commented Oct 29, 2015

Ping me when this is rebased after the other commits get merged.

This was referenced Oct 29, 2015
Merged
Merged
@tseaver tseaver mentioned this pull request Nov 6, 2015
Closed
5 tasks
@tseaver
Copy link
Contributor Author

tseaver commented Nov 16, 2015

@dhermes rebased after #1208 merged.

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

Just want to double check that 11 commits is intended after the rebase and we didn't confuse git?

@tseaver
Copy link
Contributor Author

tseaver commented Nov 16, 2015

This is the correct set of commits: I can squash some down, if you think it will help review better.

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

No more commits is almost certainly a good thing (easier to review).

dhermes
dhermes reviewed Nov 16, 2015
View reviewed changes
gcloud/streaming/buffered_stream.py Outdated

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

Unfortunately GitHub thinks there are too many changes to gcloud/streaming/test_transfer.py to display.

There are a couple places where you dropped with _Monkey(MUT, http_wrapper=_Dummy()):. Why?

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

Also

_Monkey(
    Request=lambda url, http_method, body: _Request(url, http_method, body)
)

seems like it could be done with

_Monkey(Request=_Request)

What am I missing?

dhermes
dhermes reviewed Nov 16, 2015
View reviewed changes
gcloud/streaming/transfer.py Outdated

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

Yay for "Give 'streaming.util' functions PEP8 names."!

dhermes
dhermes reviewed Nov 16, 2015
View reviewed changes
gcloud/streaming/util.py

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

Rut-roh, test_transfer.py is also not visible in https://github.com/tseaver/gcloud-python/commit/2414bfbbe449db06a7a1aaae314dc1d144a24a4b

dhermes
dhermes reviewed Nov 16, 2015
View reviewed changes
gcloud/streaming/transfer.py

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

Other than getting at the code GitHub won't show me, I've reviewed them all and made my comments.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 17, 2015

There are a couple places where you dropped with _Monkey(MUT, http_wrapper=_Dummy()):. Why?

In that same commit, transfer begins importing names directly, rather than via http_wrapper.<name>.

@dhermes
Copy link
Contributor

dhermes commented Nov 18, 2015

Seems like adding __doc__ to the named tuple and then me reviewing test_transfer.py is what remains. Am I missing anything else?

@tseaver
Copy link
Contributor Author

tseaver commented Nov 18, 2015

(I think) to review the bigger diff:

$ git remote add tseaver https://github.com/tseaver/gcloud-python
$ git fetch --all tseaver
$ git diff master..tseaver/streaming-strip_unused_features gcloud/streaming/test_transfer.py

tseaver added 17 commits November 18, 2015 16:11
@tseaver
Unused imports.
a1a43c1
@tseaver
Make imports explicit, and document them.
e74b5ae
@tseaver
Give 'streaming.util' functions PEP8 names.
37f9ed3
@tseaver
Give 'streaming.http_wrapper' functions PEP8 names.
beb8b84
@tseaver
Further import cleanup / documentation.
b6b764d
@tseaver
Remove deleted APIs from 'streaming.transfer.__all__'.
2cf253b
@tseaver
Make 'streaming.transfer.RESUMABLE_UPLOAD_THRESHOLD' public.
bf3f706
@tseaver
Adjust visibility / names of 'streaming.http_wrapper' helpers / const…
3033ca6 
…ants.
@tseaver
Strip out unused/redundant exception types.
54cb870
@tseaver
Give 'streaming.transfer' methos PEP8 names.
e22993e
@tseaver
Fix coverage gap from branch split.
73c3282
@tseaver
Remove pointless lambda wrapping '_Request'.
7ad47bb 
Addresses:
#1209 (comment)
@tseaver
Drop notes-to-self from module docstrings.
8caed56 
Addresses:
#1209 (comment)
@tseaver
Drop legacy '__all__'.
94fc9bf 
Addresses:
#1209 (diff)
@tseaver
Typo.
42ded7d 
Addresses:
#1209 (comment)
@tseaver
Move quasi-docstring comments into docstring for '_ExceptionRetryArgs'.
f66f1b8 
Addresses:
#1209 (comment)
@tseaver
Move nested '_process_content_range' helper out to module scope.
7e94700 
Addresses:
#1209 (comment)
@tseaver
Copy link
Contributor Author

tseaver commented Nov 18, 2015

Rebased to fix conflict on gcloud/storage/test_blob.py (from #1222).

@dhermes
Copy link
Contributor

dhermes commented Nov 18, 2015

OK the test_transfer.py diff looks fine (I don't know why GitHub wouldn't show it, it was mostly renames)

@dhermes
Copy link
Contributor

dhermes commented Nov 18, 2015

Do we have everything else squared away?

@tseaver
Copy link
Contributor Author

tseaver commented Nov 19, 2015

Do we have everything else squared away?

I think so.

@dhermes
Copy link
Contributor

dhermes commented Nov 19, 2015

OK. LGTM

tseaver added a commit that referenced this pull request Nov 19, 2015
@tseaver
Merge pull request #1209 from tseaver/streaming-strip_unused_features
509564d 
Streaming: strip unused features
@tseaver tseaver merged commit 509564d into googleapis:master Nov 19, 2015
@tseaver tseaver deleted the streaming-strip_unused_features branch November 19, 2015 01:00
@dhermes dhermes mentioned this pull request Jan 25, 2016
Merged
parthea pushed a commit that referenced this pull request Nov 24, 2025
@renovate-bot
chore(deps): update dependency setuptools to v60.9.2 (#1209)
5103fba 
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [setuptools](https://togithub.com/pypa/setuptools) | `==60.9.1` -> `==60.9.2` | [![age](https://badges.renovateapi.com/packages/pypi/setuptools/60.9.2/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/setuptools/60.9.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/setuptools/60.9.2/compatibility-slim/60.9.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/setuptools/60.9.2/confidence-slim/60.9.1)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>pypa/setuptools</summary>

### [`v60.9.2`](https://togithub.com/pypa/setuptools/blob/HEAD/CHANGES.rst#v6092)

[Compare Source](https://togithub.com/pypa/setuptools/compare/v60.9.1...v60.9.2)

Misc
^^^^

-   [#&#8203;3035](https://togithub.com/pypa/setuptools/issues/3035): When loading distutils from the vendored copy, rewrite `__name__` to ensure consistent importing from inside and out.

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-generator-python).
parthea pushed a commit that referenced this pull request Nov 26, 2025
@BigTailWolf
chore: update token (#1209)
8d63bd9 
* chore: update token

* pin the cryptography library version

* pin the cryptography library version

* using less operator to try
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tseaver @dhermes @googlebot