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

Set the local time zone as the default #43

Merged
merged 6 commits into from
May 13, 2014

Conversation

benfitzpatrick
Copy link
Contributor

This change allows the TimePointParser class to cast date/time
representations with unknown time zones into the local
time zone by default (although there are two switches,
assume_utc and assume_unknown_time_zone that switch off
this behaviour). Previously, they became UTC by default, which
goes against the spirit of the standard.

This change should be merged after #42.

@arjclark, please review.

@benfitzpatrick benfitzpatrick added this to the next-release milestone May 6, 2014
@benfitzpatrick
Copy link
Contributor Author

Slightly altering this now to allow the assumption of any given time zone, not
just UTC.

@benfitzpatrick
Copy link
Contributor Author

Now OK for review again.

@benfitzpatrick
Copy link
Contributor Author

#42 change merged in and re-tested.

@@ -139,12 +148,14 @@ class TimePointParser(object):
def __init__(self, num_expanded_year_digits=2,
allow_truncated=False,
allow_only_basic=False,
assume_utc=False,
assumed_time_zone=None,
no_assume_local_time_zone=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having difficulty understanding what it is that this setting is relating to. I think the logic/naming convention mismatch here is unhelpful from a readability point of view. Could you change it to assume_local_time_zone and flip logic accordingly, or something to that effect?

@arjclark
Copy link
Contributor

arjclark commented May 9, 2014

Overall looks technically sound. A few comments about naming and consistency for easy readability of code.

@benfitzpatrick
Copy link
Contributor Author

Updated

@arjclark
Copy link
Contributor

Shouldn't you be on holiday right now!

@benfitzpatrick
Copy link
Contributor Author

My finger slipped

@arjclark
Copy link
Contributor

Repeatedly and in all the "right" places to address my feedback.

@arjclark
Copy link
Contributor

Looks good. Tests ok in my environment (apart from the length of time required to run them!).

arjclark added a commit that referenced this pull request May 13, 2014
@arjclark arjclark merged commit 3bc67e0 into metomi:master May 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants