Skip to content
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.

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

Removing use of pytz module / implementing our own UTC. #1044

Merged
merged 1 commit into from
Aug 10, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 7, 2015

This was inspired by #1009.

Our methods for signing strings with credentials / crypto will likely be moved into oauth2client, so this is a pre-emptive move to avoid having pytz as a dependency for oauth2client.

As an added benefit, we avoid having pytz as a dependency, which helps for people wanting to use gcloud-python on App Engine.

/cc @nathanielmanistaatgoogle

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 7, 2015
NOW = datetime.datetime(2015, 7, 28, 16, 34, 47, tzinfo=eastern)
EPOCH = datetime.datetime(1970, 1, 1, tzinfo=pytz.utc)

class CET(_UTC):

This comment was marked as spam.

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented Aug 7, 2015

No problem with this one overall. I wonder if our local UTC should be an alias of pytz.utc, IFF pytz is importable (to avoid having more than one global for the zone).

@dhermes
Copy link
Contributor Author

dhermes commented Aug 7, 2015

@tseaver RE: default to using pytz, default to ours. Good call!

Code reviews are so useful 😄 I totally overlooked that and it's totally the right way to go.

@tseaver
Copy link
Contributor

tseaver commented Aug 7, 2015

I figured that dropping pytz was really FBO users who never care about timezones at all: those who do will want it, anyway.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 7, 2015

Yeah for sure.

@dhermes dhermes force-pushed the remove-pytz-dependency branch from 790e798 to b8bce16 Compare August 8, 2015 00:35
@dhermes
Copy link
Contributor Author

dhermes commented Aug 8, 2015

@tseaver I re-pushed the initial commit. Sorry I finished it awhile ago but forgot to push.

Wanted to actually test the case that pytz was installed, but it was being too big a PITA to deal with sys.modules. WDYT?

@tseaver
Copy link
Contributor

tseaver commented Aug 8, 2015

We could add a tox stanza that didn't install it.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 8, 2015

What is that?

@tseaver
Copy link
Contributor

tseaver commented Aug 8, 2015

Oops, I inverted it in my mind: we could add a stanza that tests it w/ pytz installed, e.g.:

[testenv:py27_w_extras]
deps =
    {[testenv]deps}
    pytz

@dhermes
Copy link
Contributor Author

dhermes commented Aug 8, 2015

Can we punt on it for this PR and fix address it after?

@tseaver
Copy link
Contributor

tseaver commented Aug 8, 2015

Sure.

The only thing I see which might be gating is we have no explicit test coverage for the _UTC class: do we need it? If not, go ahead and merge.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 8, 2015

I added it to test__helpers.py, been in here for awhile.

@tseaver
Copy link
Contributor

tseaver commented Aug 10, 2015

LGTM

dhermes added a commit that referenced this pull request Aug 10, 2015
Removing use of pytz module / implementing our own UTC.
@dhermes dhermes merged commit 99e04fd into googleapis:master Aug 10, 2015
@dhermes dhermes deleted the remove-pytz-dependency branch August 10, 2015 14:40
from gcloud import _helpers as MUT

klass = self._getTargetClass()
self.assertTrue(isinstance(MUT.UTC, klass))

This comment was marked as spam.

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants