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

Deprecate tz format #606

Merged
merged 3 commits into from
Jan 4, 2018
Merged

Conversation

pganssle
Copy link
Member

@pganssle pganssle commented Jan 3, 2018

Fixes #595.

This is a release blocker, so we should get this out ASAP.

CC: @pablogsal

@pganssle pganssle added this to the 2.7.0 milestone Jan 3, 2018
@pablogsal
Copy link
Contributor

I will take a look today at this when I get home.

@@ -1328,7 +1326,7 @@ Check the daylight limit.
'EDT'
>>> datetime(2003, 10, 26, 0, 59, tzinfo=tz).tzname()
'EDT'
>>> datetime(2003, 10, 26, 1, 00, tzinfo=tz).tzname()
>>> datetime(2003, 10, 26, 2, 00, tzinfo=tz).tzname()
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this indicates we're not testing the doctests. Probably we should figure out how to mock out now in doctests so that we can actually get passing doctests.

Copy link
Contributor

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Apart from the suggestion, LGTM

'is now deprecated; support for parsing this format ' +
'will be removed in future versions. It is recommended ' +
'that you switch to a standard format like the GNU ' +
'TZ variable format.', tz.DeprecatedTzFormatWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Include link to the GNU TZ variable format either withing dateutil documentation or to the format specification.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea, and one I considered. I think it is in the tzstr documentation. I'm a bit hesitant to add it here, since I'm afraid of it going out of date. I'll merge this for now, we can tweak the message later.

@pganssle pganssle merged commit 643c98d into dateutil:master Jan 4, 2018
@pganssle pganssle mentioned this pull request Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants