From: Aymeric Augustin <aymeric.augus...@polytechnique.org>
Date: Thu, 10 Nov 2011 08:46:01 +0100
Local: Thurs, Nov 10 2011 2:46 am
Subject: Re: Request for review / Multiple time zone support for datetime representation
Thanks for the review! I've updated the branch with your suggestions.
Here are a few comments -- when I have something to say other than
2011/11/8 Luke Plant <L.Plant...@cantab.net>:
> == django/contrib/admin/util.py ==It's a bug: the first elif should test for models.DateTimeField. The
>> elif isinstance(field, models.DateField):
> Isn't the first clause in the second elif rendered useless by the
problem didn't appear in the tests because DateTimeField is a subclass
> == django/db/backends/mysql/base.py ==I'm now checking if the value is naive before overwriting its tzinfo.
>> # Finally, MySQLdb always returns naive datetime objects.
> Is it possible that a future version of MySQLdb could change this? If
If the database adapter returns an aware datetime (in a future
version), it's passed through unchanged.
> There is a similar issue in the Oracle backend I think.The issue existed in SQLite, MySQL and Oracle.
>> raise ValueError("MySQL backend does not support timezone-awareI've updated the message for SQLite, MySQL and Oracle.
> This seems to be misleading - shouldn't the message add the caveat "if
> == django/db/backends/sqlite3/base.py ==All backends but PostgreSQL have supports_timezones = False. It means
>> supports_timezones = False
> What exactly is meant by this? The timezone support in the sqlite
> If SQLite genuinely does not support time zones in some important way,
that the database engine (SQLite, MySQL) or its adapter (cx_Oracle)
doesn't support timezones. This flag existed before my branch, and it
was True for SQLite. I switched it to False because SQLite doesn't
actually support timezones. This shouldn't affect anything besides
Django's own test suite -- database features aren't part of the public
I've added a paragraph about this change to the release notes.
> == django/utils/timezone.py ==This code is taken straight from Python's docs, and I wanted to keep
>> import time as _time
> There doesn't seem to be any clash to avoid, so I don't see the need for
it as close as possible to the original for clarity and ease of
maintenance -- see #16899 for example.
I can't use the original code as is, because the timezone isn't known
> However, my biggest question is about the LocalTimezone class. We nowYes. I spent a lot of time on this problem, and I eventually decided
> have two implementations of this, one in django.utils.tzinfo, which
> seems to have more extensive tests, and a new one in
> django.utils.timezone. The old one is still in use, both by old code
> e.g. in django/contrib/syndication/views.py and new code e.g.
> django/utils/dateformat.py under DateFormat.__init__
> The new one is used in other places.
to add a second implementation of the LocalTimezone, because there are
too many issues in the "old" implementation to build more code upon
it. The "new" implementation focuses on correctness and simplicity.
Here are the problems of the "old" implementation.
Its __init__ method takes an argument, which is problematic for
> Special requirement for pickling: A tzinfo subclass must have an __init__() method that can be called with no arguments, else it can be pickled but possibly not unpickled again. This is a technical requirement that may be relaxed in the future.Adding a __getinitargs__ method resolved this problem, but it's magic,
and we're still doing something that's officially discouraged.
Also, I don't like the hack in the "old" implementation to support
This inaccuracy is irrelevant for syndication (dates are in the past)
> Perhaps we just need to combine the two LocalTimezone implementations inThere are two problems in deprecating the "old" implementation:
> tzinfo.py? If we can't do that for some backward compatibility reasons,
> can we have some explanation, and preferably a deprecation path for the
> older code? Finally, do we need to address code that uses the old
> LocalTimezone in some way - should it be unconditionally using our own
> LocalTimezone instead of something from pytz when available?
- the output of self.tzname() would change — it depends on the
argument taken by __init__;
- post-2037 dates would no longer be supported.
To sum up:
> There is no explanation of the need for these two, or when they shouldI've added a comment.
> be used, which to me seems like a minimum requirement.
>> - It includes the time zone for aware datetime objects. It raisesThe current wording describes what my branch does:
>> an exception for aware time objects.
> First instance of 'aware' should be 'naive'. Second sentence should end
- naive datetime => output without TZ info
- aware datetime => output with TZ info
- naive time => output without TZ info
- aware time => exception
> == tests ==You apparently ran out of energy mid-sentence :) Do you remember what
> I haven't actually reviewed any of the test code - I ran out of energy,
> I did run coverage for the tests and found:
> - without pytz installed, the new LocalTimezone class
you were going to say?
Thanks again for the review!
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.