Hi Luke,
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
"fixed, thanks".
2011/11/8 Luke Plant <l.plant...@cantab.net>:
> == django/contrib/admin/util.py ==
>
>> elif isinstance(field, models.DateField):
>> return formats.localize(timezone.aslocaltime(value))
>> elif isinstance(field, models.DateField) or isinstance(field,
> models.TimeField):
>> return formats.localize(value)
>
> Isn't the first clause in the second elif rendered useless by the
> previous elif?
It's a bug: the first elif should test for models.DateTimeField. The
problem didn't appear in the tests because DateTimeField is a subclass
of DateField.
> == django/db/backends/mysql/base.py ==
>
>> # Finally, MySQLdb always returns naive datetime objects.
>
> Is it possible that a future version of MySQLdb could change this? If
> so, would it be better to not make this assumption, and future proof the
> code? We could perhaps just add an assertion for the assumption we make,
> so that we don't end up silently corrupting data with a future version
> of MySQLdb that acts differently, because at the moment the function
> datetime_or_None_with_timezone_support just wipes out any existing
> tzinfo field.
I'm now checking if the value is naive before overwriting its tzinfo.
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-aware
> datetimes.")
>
> This seems to be misleading - shouldn't the message add the caveat "if
> USE_TZ == False" or something?
I've updated the message for SQLite, MySQL and Oracle.
> == django/db/backends/sqlite3/base.py ==
>
>> supports_timezones = False
>
> What exactly is meant by this? The timezone support in the sqlite
> backend appears to be very similar to that in MySQL.
>
> If SQLite genuinely does not support time zones in some important way,
> this should probably be noted in the release notes, and definitely in
> docs/ref/databases.txt
All backends but PostgreSQL have supports_timezones = False. It means
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
API (AFAIK).
I've added a paragraph about this change to the release notes.
> == django/utils/timezone.py ==
>
>> import time as _time
>
> There doesn't seem to be any clash to avoid, so I don't see the need for
> importing like this.
This code is taken straight from Python's docs, and I wanted to keep
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
at compile time, only after the settings have been loaded. But I still
think it's worth minimizing the diff.
> However, my biggest question is about the LocalTimezone class. We now
> 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.
Yes. I spent a lot of time on this problem, and I eventually decided
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
pickling and deepcopying. From Python's docs:
> 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
post-2037 dates. Sure, it doesn't crash, but it can return wrong data,
because DST doesn't apply during the same period every year. So much
for refusing the temptation to guess in the face of ambiguity :)
This inaccuracy is irrelevant for syndication (dates are in the past)
and acceptable for display purposes in dateformat (naturaltime will
return "in X years", an offset of 1 hour doesn't matter). But when
we're saving data in the database, we can't take the risk to corrupt
it.
> Perhaps we just need to combine the two LocalTimezone implementations in
> 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?
There are two problems in deprecating the "old" implementation:
- 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:
- the "old" implementation isn't suitable for the needs of my branch;
- there's a lot of history, and we can't deprecate it without creating
regressions:
https://code.djangoproject.com/log/django/trunk/django/utils/tzinfo.py
> There is no explanation of the need for these two, or when they should
> be used, which to me seems like a minimum requirement.
I've added a comment.
>> - It includes the time zone for aware datetime objects. It raises
>> an exception for aware time objects.
>
> First instance of 'aware' should be 'naive'. Second sentence should end
> 'aware datetime or time objects' for completeness.
The current wording describes what my branch does:
- naive datetime => output without TZ info
- aware datetime => output with TZ info
- naive time => output without TZ info
- aware time => exception
> == tests ==
>
> I haven't actually reviewed any of the test code - I ran out of energy,
> and test code is so dull, sorry!
>
> I did run coverage for the tests and found:
>
> - without pytz installed, the new LocalTimezone class
You apparently ran out of energy mid-sentence :) Do you remember what
you were going to say?
Thanks again for the review!
--
Aymeric.
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
