#847: prepare for using connection as context mgr

[tseaver]

Normalize patterns across three API subpackages:

• Make the threadlocal stack part of the related connection module.
• Move the get_connection() convenience API up to the base subpackage __init__.
• Move the implementation of _require_connection down into the related _implicit_environ module.

[coveralls]

Coverage remained the same at 100.0% when pulling 710af73 on tseaver:847-connection_as_context_mgr-setup into 6bc4e02 on GoogleCloudPlatform:master.

[jgeewax]

I haven't read through this code yet, but my assumption was that, given the big debate on that other issue regarding the client-pattern, it seemed we had a lot of other people agreeing that it would be worth exploring that direction.

Is that not the case ? Did we decide somewhere to go the context-manager pattern?

[dhermes]

@tseaver I'm in the same boat with @jgeewax

[tseaver]

I may have lost track of some that thread, but still don't think my concerns about the "client" pattern got addressed.

I wanted to line ducks up to allow demonstrating the context manager pattern. These cleanups / normalizations are intended to help that, but they don't implement it, and should be neutral for either one.

[tseaver]

ISTM that a 100+ comment argument thread on Github is probably the wrong way to try to resolve such an issue.

[dhermes]

@tseaver Do we still want this?

[tseaver]

@dhermes I think so. Moving the get_connection() / _require_connection() bits into the same places in each sub-package is a definite win. Moving the "stack" into the connection module is maybe more debatable, but still seems right to me, even if we don't continue on to allow using connections directly as context managers.

[dhermes]

Let's break off the definite win as a single PR and then go from there. I'm finding this hard to digest as one piece.

[tseaver]

@dhermes Dropping f8e40d7 isn't that simple -- it also breaks what would be import cycles between datastore.batch <--> datastore._implicit_environ and storage.batch <--> storage._implicit_environ.

[dhermes]

I'm not seeing the value to be worth the struggle.

[tseaver]

Having the get_connection and _require_connection functions in sensible / consistent locations isn't valuable? Their current locations are purely happenstance, and would make no sense to our future selves.

Setting up the connection stack to be non-batch specific can't hurt, and it does allow the batch modules not to be pulled into the _require_connection dance.

[dhermes]

• I agree with you that it's "valuable" but the hoop jumping in this PR is just a lot to bear. (More than the value added). Batch.top() is a useful concept
• For our users, we put them in the sub-package, so they don't care.
• If we're worried about maintainers, we can put a code comment explaining the reason
• When we fully transition to clients, we may decide to ditch all the implicit stuff, so _implicit_environ will just go away

[dhermes]

@tseaver is this dead?