Streaming: strip unused features #1209

tseaver · 2015-10-29T00:29:03Z

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.

dhermes · 2015-10-29T18:11:21Z

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

tseaver · 2015-11-16T21:29:28Z

@dhermes rebased after #1208 merged.

dhermes · 2015-11-16T21:49:01Z

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

tseaver · 2015-11-16T22:11:57Z

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

dhermes · 2015-11-16T22:28:24Z

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

gcloud/streaming/buffered_stream.py

@@ -2,6 +2,10 @@
 """Small helper class to provide a small slice of a stream.

 This class reads ahead to detect if we are at the end of the stream.
+
+:mod:`gcloud._apidools.transfer` uses:


dhermes · 2015-11-16T22:34:48Z

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 · 2015-11-16T22:37:21Z

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?

gcloud/streaming/transfer.py

+- :class:`Upload`
+- :const:`RESUMABLE_UPLOAD`
+
+:mod:`gcloud.storage.test_blob` patches:


dhermes · 2015-11-16T22:43:34Z

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

gcloud/streaming/util.py



-def Typecheck(arg, arg_type, msg=None):
+def type_check(arg, arg_type, msg=None):


dhermes · 2015-11-16T22:57:11Z

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

gcloud/streaming/transfer.py

          use_chunks: (bool, default: True) If False, ignore self.chunksize
              and stream this download in a single request.

        Returns:
            None. Streams bytes into self.stream.
        """
-        self.EnsureInitialized()
+        self._ensure_initialized()
        while True:


dhermes · 2015-11-16T23:05:26Z

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

tseaver · 2015-11-17T00:46:53Z

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>.

tseaver · 2015-11-17T01:37:35Z

But http_wrapper=_Dummy() wasn't actually replacing anything. So why was it there and why was it safe to remove the monkey patch?

Originally, the _Dummy() had the names which transfer expected to find on http_wrapper: over the course (and after I started importing them directly), it "withered away" like the Communist state.

dhermes · 2015-11-17T01:40:04Z

Ha. OK

dhermes · 2015-11-18T19:50:34Z

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

tseaver · 2015-11-18T20:25:03Z

(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

…ants.

Addresses: #1209 (comment)

Addresses: #1209 (diff)

Addresses: #1209 (comment)

tseaver · 2015-11-18T21:13:32Z

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

dhermes · 2015-11-18T21:48:17Z

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

dhermes · 2015-11-18T21:48:29Z

Do we have everything else squared away?

tseaver · 2015-11-19T00:40:17Z

Do we have everything else squared away?

I think so.

dhermes · 2015-11-19T00:45:34Z

OK. LGTM

Streaming: strip unused features

tseaver added hygiene api: core labels Oct 29, 2015

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

This was referenced Oct 29, 2015

Streaming: add pep8 / pylint goodness #1210

Merged

Add coverage for 'gcloud._apitools.transfer' #1207

Merged

tseaver mentioned this pull request Nov 6, 2015

Re-vendor / cleanup apitools as used in storage #1186

Closed

5 tasks

dhermes reviewed Nov 16, 2015
View reviewed changes

gcloud/streaming/transfer.py

- :class:`Upload`

- :const:`RESUMABLE_UPLOAD`

:mod:`gcloud.storage.test_blob` patches:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes reviewed Nov 16, 2015
View reviewed changes

gcloud/streaming/util.py

def Typecheck(arg, arg_type, msg=None):

def type_check(arg, arg_type, msg=None):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes reviewed Nov 16, 2015
View reviewed changes

tseaver added 17 commits November 18, 2015 16:11

Unused imports.

a1a43c1

Make imports explicit, and document them.

e74b5ae

Give 'streaming.util' functions PEP8 names.

37f9ed3

Give 'streaming.http_wrapper' functions PEP8 names.

beb8b84

Further import cleanup / documentation.

b6b764d

Remove deleted APIs from 'streaming.transfer.__all__'.

2cf253b

Make 'streaming.transfer.RESUMABLE_UPLOAD_THRESHOLD' public.

bf3f706

Adjust visibility / names of 'streaming.http_wrapper' helpers / const…

3033ca6

…ants.

Strip out unused/redundant exception types.

54cb870

Give 'streaming.transfer' methos PEP8 names.

e22993e

Fix coverage gap from branch split.

73c3282

Remove pointless lambda wrapping '_Request'.

7ad47bb

Addresses: #1209 (comment)

Drop notes-to-self from module docstrings.

8caed56

Addresses: #1209 (comment)

Drop legacy '__all__'.

94fc9bf

Addresses: #1209 (diff)

Typo.

42ded7d

Addresses: #1209 (comment)

Move quasi-docstring comments into docstring for '_ExceptionRetryArgs'.

f66f1b8

Addresses: #1209 (comment)

Move nested '_process_content_range' helper out to module scope.

7e94700

Addresses: #1209 (comment)

tseaver added a commit that referenced this pull request Nov 19, 2015

Merge pull request #1209 from tseaver/streaming-strip_unused_features

509564d

Streaming: strip unused features

tseaver merged commit 509564d into googleapis:master Nov 19, 2015

tseaver deleted the streaming-strip_unused_features branch November 19, 2015 01:00

dhermes mentioned this pull request Jan 25, 2016

Upgrading version to 0.9.0. #1418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streaming: strip unused features #1209

Streaming: strip unused features #1209

tseaver commented Oct 29, 2015

dhermes commented Oct 29, 2015

tseaver commented Nov 16, 2015

dhermes commented Nov 16, 2015

tseaver commented Nov 16, 2015

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

tseaver commented Nov 17, 2015

tseaver commented Nov 17, 2015

dhermes commented Nov 17, 2015

dhermes commented Nov 18, 2015

tseaver commented Nov 18, 2015

tseaver commented Nov 18, 2015

dhermes commented Nov 18, 2015

dhermes commented Nov 18, 2015

tseaver commented Nov 19, 2015

dhermes commented Nov 19, 2015



		def Typecheck(arg, arg_type, msg=None):
		def type_check(arg, arg_type, msg=None):

Streaming: strip unused features #1209

Streaming: strip unused features #1209

Conversation

tseaver commented Oct 29, 2015

dhermes commented Oct 29, 2015

tseaver commented Nov 16, 2015

dhermes commented Nov 16, 2015

tseaver commented Nov 16, 2015

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Nov 16, 2015

tseaver commented Nov 17, 2015

tseaver commented Nov 17, 2015

dhermes commented Nov 17, 2015

dhermes commented Nov 18, 2015

tseaver commented Nov 18, 2015

tseaver commented Nov 18, 2015

dhermes commented Nov 18, 2015

dhermes commented Nov 18, 2015

tseaver commented Nov 19, 2015

dhermes commented Nov 19, 2015