Large number of style-related changes to pass pylint

[dhermes]

This is not to be merged, but should be reviewed and scrutinized. /cc @silvolu @tseaver

This is upstream of #235 as that was motivated by this larger effort towards fixing #179 and #178.

As for review, we should particularly discuss the things I pointed out.

Some key things to notice:

• The tools (pep8 / pylint) disagree about continutation lines.
  This is because PEP8 is ambiguous about "multi-line constructs".
  One of the examples for bracket indentation in the PEP is

  my_list = [
      1, 2, 3,
      4, 5, 6,
      ]

  but the PEP doesn't explicity say whether or not

  my_list = [1, 2, 3,
             4, 5, 6,
             ]

  would be allowed. As it turns out, pep8 (the tool) considers
  the latter to be a continuation while pylint does not.

• I changed all indentation where this was a problem, but was
  intentionally inconsistent. This way we can see "all options"
  and discuss whichever is preferred.

• I made a _MetadataMixin helper class for methods shared between
  storage.Bucket and storage.Key. I did not refactor the affected
  tests but they do still give 100% coverage.

• To silence docstrings warnings I added many bogus docstrings
  without actually documenting the given objects.

• I factored out part of

  storage.connection.Connection.generate_signed_url

  into a _get_expiration_seconds() method and added tests.
  This method is untested (has # pragma NO COVER) and is still very long.
  I refactored because pylint complained about too many locals.

• I made sure tox -e lint and tox -e cover passed.

• I increased the maximum number of arguments from 5 to 10 and
  the maximum public methods from 20 to 30. Offenders were:

  • storage.connection.Connection.make_request (6 arguments)
  • storage.connection.Connection.api_request (9 arguments)
  • storage.connection.Connection.generate_signed_url (6 arguments)
  • storage.bucket.Bucket (27 public methods) and
  • storage.key.Key (23 public methods)

  I think refactoring these should be considered (though is not
  mandatory). I certainly don't know a good solution for the class
  public methods.

[dhermes]

@tseaver I also forgot to mention that I needed a mock for datetime.datetime.utcnow in test__get_expiration_seconds (in storage/test_connection.py) .

I just did the first thing that came to mind, but it is a bit hacky. So

1. Do we have a standard mocking approach / library that we want to use.
2. Do you have any insights specific to mocking datetime. (It was kind of a pain.)

[tseaver]

Hmm, tests are failing for me (maybe a timezone-related issue?):

py27 create: /home/tseaver/projects/agendaless/Google/src/gcloud-python/.tox/py27
py27 installdeps: nose, unittest2
py27 inst: /home/tseaver/projects/agendaless/Google/src/gcloud-python/.tox/dist/gcloud-0.02.2.zip
py27 runtests: PYTHONHASHSEED='3555183268'
py27 runtests: commands[0] | nosetests
.....................................................................................................................................................................................................................................................................................................FF..............................................................................................................
======================================================================
FAIL: test__get_expiration_seconds (gcloud.storage.test_connection.TestConnection)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/gcloud/storage/test_connection.py", line 526, in test__get_expiration_seconds
    CALL_FUT(expiration_no_tz))
AssertionError: 1092902400 != 1092891600

======================================================================
FAIL: test__get_expiration_seconds_w_timedelta (gcloud.storage.test_connection.TestConnection)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/gcloud/storage/test_connection.py", line 583, in test__get_expiration_seconds_w_timedelta
    CALL_FUT(expiration_as_delta))
AssertionError: 1092902400 != 1092891600

[dhermes]

@tseaver I think I don't understand the expected behavior well enough. You'll notice the difference is 3 hours, the difference between our local time zones. This must mean the relevant code

expiration.replace(tzinfo=pytz.utc)

is actually assuming the datetime.datetime object has the local timezone?

[tseaver]

WRT mocking datetime, I've done the following (e.g., at module scope of the MUT):

_UTCNOW = None # unit tests can replace
def _utcnow(): #pragma NO COVERAGE
    if _UTCNOW is not None:
        return _UTCNOW
    return datetime.datetime.utcnow()

Later in the module, replace 'datetime.datetime.utcnow()' with '_utcnow()'.

and then in a test:

def test_foo(self):
    import datetime
    from gcloud import credentials as MUT

    utcnow = datetime.datetime.utcnow()
    with _Monkey(MUT, _UTCNOW=utcnow):
        result = self._callFUT(1000)
    self.assertEqual(result, utcnow + datetime.timedelta(0, 1000)

[dhermes]

Rebased to master after #235 was merged.

[dhermes]

@tseaver Since datetime.datetime is not a pure Python class, the attributes can't be patched.

>>> import datetime
>>> datetime.datetime.utcnow = None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't set attributes of built-in/extension type 'datetime.datetime'

[dhermes]

BTW @tseaver this PR can't be merged until we resolve the discussion, so the rebase is not so important.

Here are the datastore docstrings for you to handle:
https://gist.github.com/dhermes/ab246529ad1e2ae0b9bf

These are the rest, I will tackle:
https://gist.github.com/dhermes/a52007c58557419a310d

[dhermes]

Found the issue and it wasn't with datetime. I'm happy we tested this because time.mktime computes a local time, which the method goes out of it's way to avoid. See StackOverflow for details.

[tseaver]

@dhermes I wasn't suggesting patching the 'datetime.datetime' class: I was patching the 'gcloud.credentials' module.

[coveralls]

Coverage remained the same when pulling 111ce66 on dhermes:large-style-changes into 61232de on GoogleCloudPlatform:master.

[dhermes]

@tseaver I see now, sorry I read too quickly.

[dhermes]

NOTE: I just rebased on top of #241

[coveralls]

Coverage remained the same when pulling af1d5d7 on dhermes:large-style-changes into 61232de on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same when pulling af1d5d7 on dhermes:large-style-changes into 61232de on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same when pulling 936ff32 on dhermes:large-style-changes into 61232de on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same when pulling 9fa9d90 on dhermes:large-style-changes into 61232de on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same when pulling 76a8d05 on dhermes:large-style-changes into 11c8765 on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same when pulling f2bef6e on dhermes:large-style-changes into fba037f on GoogleCloudPlatform:master.