Lazy load defaults

[dhermes]

@tseaver This can be split into 3 distinct PRs if you like:

• Use an Environment class and an instance in _implicit_environ (attrs of instance replace class "constants")
• Moving the code for set_default_dataset_id from __init__ to _implicit_environ
• Add a lazy loading descriptor for setting the default dataset ID on the default instance from _implicit_environ

Still missing: a lazy loading descriptor for connection (it's pretty tangled in to the tests, so I wanted to discuss this before putting in too much work on that)

UPDATE: I think the best strategy for testing might be to just replace _implicit_environ.DEFAULT_ENVIRON with a fake instance that has Nones explicitly set in place of the lazily loaded pieces

[coveralls]

Coverage remained the same at 100.0% when pulling 662ad4a on dhermes:lazy-load-defaults into 8e68a20 on GoogleCloudPlatform:master.

[dhermes]

@jgeewax This begins to cover one of the concerns in #647.

Also note that this adds a fair amount of complexity to our codebase all for the purposes of letting people forego

datastore.set_defaults()

FWIW I'd really like to be able for this to "just work" but it will involve a lot of actual work.

[jgeewax]

I'm a huge fan of your "UPDATE" note above... Wouldn't it be easier to just create an Environment... and then make the dataset field a property method, that calls get_default_dataset_id when it's needed?

By that I mean...

def get_current_environment():
  if current_environment:
    return current_environment
  else:
    return Environment(dataset_id=whatever is set by default, connection=whatever is set by default)

# ... later ...

def do_something_needing_environment(self):
  env = get_current_environment()
  env.connection.send_request(...)

[dhermes]

@jgeewax You want the environment to be long lived because all the checks require I/O of some kind, so do it once and never again. Also, my comment was just about testing, where the assumption in most places was that these values would return None and not try to lazily load.

[tseaver]

LGTM

[dhermes]

@tseaver I'm hesitant to merge, would like to discuss.

I've been hacking on getting the lazy loaded connection and have realized

• We don't want lazy loading in tests (unless we are testing lazy loading)
• Every time a fresh DEFAULT_ENVIRON object is _Monkey-patched in a test, the property is lazy loaded
• If the modules import the DEFAULT_ENVIRON object, patching it on the _implicit_environ namespace doesn't change the object that has been imported
• Being super careful to only import the _implicit_environ module (instead of the object) is not future proof

I am debating a base class that explicitly sets the attribute to None:

class DatastoreBaseTest(unittest2.TestCase):

    def setUp(self):
        from gcloud.datastore import _implicit_environ
        self._replaced_default = _implicit_environ.DEFAULT_ENVIRON
        _implicit_environ.DEFAULT_ENVIRON = _implicit_environ.Environment()
        _implicit_environ.DEFAULT_ENVIRON.dataset_id = None

    def tearDown(self):
        from gcloud.datastore import _implicit_environ
        _implicit_environ.DEFAULT_ENVIRON = self._replaced_default

but this is again not future proof since it requires tests to inherit from it.

[dhermes]

@tseaver Maybe we can put the contents of _implicit_environ.py in dataset.py and most of the stuff in __init__.py as well? We can use a default Dataset object in there, i.e. dataset._DEFAULT_ENVIRON. But users will only see

get_default_dataset_id()
set_default_dataset_id()
get_default_connection()
set_default_connection()
set_defaults()
# get_defaults() would return that object, should we allow to use at their own risk?

[dhermes]

Basic design (to add to Dataset) after #660 is in:
https://gist.github.com/dhermes/36eb807880718703cb9e

Then at the bottom of dataset.py create

_DEFAULT_DATASET = Dataset(_implicit=True)

def get_default_dataset_id():
    return _DEFAULT_DATASET.dataset_id

...

and in tests manually set

dataset._DEFAULT_DATASET.dataset_id = None
dataset._DEFAULT_DATASET.connection = None

either in setupModule or setupClass (so the lazy loading does not occur).

[tseaver]

Wouldn't this be simpler?

with _Monkey(dataset, _DEFAULT_DATASET=Dataset('DATASET_ID', connection)):
       ...

[dhermes]

If every test needs do that to avoid lazyily loading by mistake, it will become quite cumbersome. Though you're right that the _DEFAULT_DATASET object should be replaced wholesale.

[dhermes]

Arg! After #660 there is really nowhere to put a Dataset object since it creates cycles everywhere that _implicit_environ.CONNECTION (e.g.) was used.

[jgeewax]

Just out of curiosity (I absolutely could be oversimplifying here) but are we abstracting this away the right way?

Looks like we have now:

_DEFAULT_DATASET = Dataset(...)

def get_default_dataset_id():
  return _DEFAULT_DATASET.dataset_id

would this be better as...

DEFAULT_DATASET_ID = None

def set_default_dataset_id(dataset_id):
  DEFAULT_DATASET_ID = dataset_id

def get_default_dataset_id():
  return DEFAULT_DATASET_ID or magic_method_to_get_my_dataset_from_appengine()

def get_default_dataset():
  return Dataset(dataset_id=get_default_dataset_id())

?

And then we never know about a special variable name, but always call the get_default_dataset() method when we need a dataset?

[dhermes]

@jgeewax We were talking about unittests.

[jgeewax]

Sorry about that -- misread. Ignore me.

[dhermes]

#663, #664, #665, #666, #667, and #668 step in for this, closing this PR