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

Fixed #22598 -- Allowed make_aware to work with ambiguous datetime #4416

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@jarshwah
Copy link
Member

jarshwah commented Mar 31, 2015

I wrote this patch before discovering the ticket (which was wontfixed), but I believe this patch takes a different approach. https://code.djangoproject.com/ticket/22598

Where the original reporter wanted to try different settings until it worked, this patch allows the user to resolve the ambiguity if they choose.

Still need to finish off the tests (and docs), and I need to work out whether or not make_naive would need similar options.

@carljm

This comment has been minimized.

Copy link
Member

carljm commented Mar 31, 2015

I'm in favor of this patch.

I think it would also be worth mentioning in the make_aware docs specifically which exception(s) it may raise on ambiguous / non-existent dates, so users have the information they would need in order to catch those exceptions and call again with is_dst set.

@aaugustin

This comment has been minimized.

Copy link
Member

aaugustin commented Apr 1, 2015

The original reasons why Aymeric-from-four-years-ago didn't include the is_dst argument are:

  • If developers are going to set is_dst, they may just as well use pytz directly instead of Django's wrappers.
  • Developers will set it randomly to True or False just to prevent their code from crashing and sometimes get incorrect data. (Pro-tip: crashing is rarely a good business outcome.)

Generally speaking I'm slightly ashamed of several design choices in the time zone APIs. I'm happy to let others improve them. So go ahead!

@jarshwah jarshwah force-pushed the jarshwah:make-aware-ambiguous branch from a8d6d2c to 0763465 Apr 21, 2015

@jarshwah

This comment has been minimized.

Copy link
Member

jarshwah commented Apr 21, 2015

I've updated the patch with some tests and docs. I don't think is_dst can be used without pytz, because I can't get an exception to be thrown when using an ambiguous date.

Does this patch go far enough? Or would you expect to see more here?

I'm also not too sure about the wording of the docs for make_aware. It covers both error conditions for when pytz is in use, but it might be a little verbose/unclear.


When ``pytz`` is available the exception ``pytz.NonExistentTimeError``
will be raised if you try to make ``value`` aware during a DST transition
such that the time never occurred (when entering into DST). You can catch

This comment has been minimized.

@carljm

carljm Apr 21, 2015

Member

This paragraph (in contrast with the above paragraph) seems to imply that the is_dst parameter is not useful for avoiding NonExistentTimeError. But it is: if you pass is_dst=[True|False] to tz.localize you will never get a NonExistentTimeError. The way it resolves isn't quite as easy to explain as with AmbiguousTimeError - effectively you either get the time an hour earlier than the nonexistent time, or an hour later, depending which value you pass for is_dst.

This comment has been minimized.

@jarshwah

jarshwah Apr 22, 2015

Member

Oh that's cool then, and solves one of the issues I had where I thought I wasn't really doing anything with regards to NonExtistentTimeError. I'll write some tests and correct the docs. Thanks for pointing that out.


@unittest.skipIf(pytz is None, "this test requires pytz")
def test_make_aware_pytz_ambiguous(self):
# 2:30 happens twice, once before DLS ends and once after

This comment has been minimized.

@carljm

carljm Apr 21, 2015

Member

isn't DST, not DLS, the standard acronym for daylight savings?

actual = timezone.make_aware(
non_existent + datetime.timedelta(hours=1), timezone=CET)

self.assertEqual(actual.hour, 3)

This comment has been minimized.

@carljm

carljm Apr 21, 2015

Member

We should also test the nonexistent time with is_dst=True and is_dst=False

@carljm

This comment has been minimized.

Copy link
Member

carljm commented Apr 21, 2015

This basically looks good to me; left a couple comments on docs and tests.

I don't think it's true to say (as you do in your comment) that you "can't use" is_dst without pytz; it is correct to say (as you do in the docs) that it will have no effect.


Returns an aware :class:`~datetime.datetime` that represents the same
point in time as ``value`` in ``timezone``, ``value`` being a naive
:class:`~datetime.datetime`. If ``timezone`` is set to ``None``, it
defaults to the :ref:`current time zone <default-current-time-zone>`.

This function can raise an exception if ``value`` doesn't exist or is
ambiguous because of DST transitions.
When ``pytz`` is available the exception ``pytz.AmbiguousTimeError``

This comment has been minimized.

@timgraham

timgraham Apr 22, 2015

Member

available -> installed? (and add a comma after whatever you choose)

This function can raise an exception if ``value`` doesn't exist or is
ambiguous because of DST transitions.
When ``pytz`` is available the exception ``pytz.AmbiguousTimeError``
will be raised if you try to make ``value`` aware

This comment has been minimized.

@timgraham

timgraham Apr 22, 2015

Member

premature line break (can fit some more words after "aware")

exception by choosing if the time is pre-transition or post-transition
respectively. ``is_dst`` has no effect when ``pytz`` is not installed.

When ``pytz`` is available the exception ``pytz.NonExistentTimeError``

This comment has been minimized.

@timgraham

timgraham Apr 22, 2015

Member

available/comma comment as above

such that the time never occurred (when entering into DST). Setting
``is_dst`` to ``True`` or ``False`` will avoid the exception by moving the
hour backwards or forwards by 1 respectively. For example, ``is_dst=True``
would change a non-existent time of 2:30 to 1:30 where ``is_dst=False``

This comment has been minimized.

@timgraham

timgraham Apr 22, 2015

Member

where -> and

@@ -154,6 +154,9 @@ Internationalization
* The :func:`django.views.i18n.javascript_catalog` view now works correctly
if used multiple times with different configurations on the same page.

* The :func:`django.utils.timezone.make_aware` gained an ``is_dst`` argument

This comment has been minimized.

@timgraham

timgraham Apr 22, 2015

Member

The make_aware function... (add "function")

@@ -154,6 +154,9 @@ Internationalization
* The :func:`django.views.i18n.javascript_catalog` view now works correctly
if used multiple times with different configurations on the same page.

* The :func:`django.utils.timezone.make_aware` gained an ``is_dst`` argument
to help users resolve ambiguous times during DST transitions.

This comment has been minimized.

@timgraham

timgraham Apr 22, 2015

Member

chop "users"

# round trip to UTC then back to CET
std = timezone.localtime(timezone.localtime(std, timezone.UTC()), CET)
dst = timezone.localtime(timezone.localtime(dst, timezone.UTC()), CET)
self.assertEqual( (std.hour, std.minute), (3, 30))

This comment has been minimized.

@timgraham

@timgraham timgraham changed the title [WIP] Fixed #22598 -- Allowed make_aware to work with ambiguous datetime Fixed #22598 -- Allowed make_aware to work with ambiguous datetime Apr 22, 2015

@jarshwah jarshwah force-pushed the jarshwah:make-aware-ambiguous branch from 4b7dbd1 to 38e405b Apr 24, 2015

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Apr 24, 2015

merged in 143255c, thanks!

@timgraham timgraham closed this Apr 24, 2015

@miraculixx

This comment has been minimized.

Copy link

miraculixx commented Oct 25, 2015

👍 any chance this could be enhanced using a new setting, so devs can decide what behavior they want without having to change existing code?

# settings.py
TIME_ZONE_LOCALIZE_DST = True  # call timezone.localize with is_dst (default), resulting the actual time during dst changes
# or
TIME_ZONE_LOCALIZE_DST = False  # call timezone.localize with is_dst (default), resulting in the time before dst changes
# updated make_aware
def make_aware(self, value, timezone):
        """
        Makes a naive datetime.datetime in a given time zone aware.
        """
        if hasattr(timezone, 'localize'):
            # available for pytz time zones
            localized = timezone.localize(value, is_dst=settings.TIME_ZONE_LOCALIZE_DST or True)
            return localized
        else:
            # may be wrong around DST changes
            return value.replace(tzinfo=timezone)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment