Hi Aymeric,
Finally got around to this! My review follows. I didn't follow the
original discussions very closely, so forgive any questions that have
been answered already. On the other hand, the code and comments probably
ought to answer the kind of questions I have, without needing to search
mailing list archives, so it might be useful that I haven't been
following that closely.
The code looks in very good shape. I've got quite a lot of things listed
below, but the vast majority of my comments are small or very small nits
that can be easily corrected, with the possible exception of my
questions regarding django.utils.timezone.LocalTimezone
== 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?
== 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.
There is a similar issue in the Oracle backend I think.
> 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?
== django/db/backends/oracle/base.py ==
> raise ValueError("Oracle backend does not support timezone-aware
datetimes.")
As above, should add "if USE_TZ == False"
> def output_type_handler(cursor, name, defaultType,
> size, precision, scale):
tiny nit - is there a reason for camelCase just for the defaultType
variable?
== 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
== 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.
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.
There is no explanation of the need for these two, or when they should
be used, which to me seems like a minimum requirement.
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?
> # This function exists for consistency with get_default_timezone_name
> def get_default_timezone_name():
I'm guessing the comment should refer to get_current_timezone_name instead?
> def is_aware()
It is probably worth adding a comment indicating that the logic is taken
straight from Python docs, with a link -
http://docs.python.org/library/datetime.html#available-types
== docs/ ==
Some instances of 'time-zone' can be found, instead of 'time zone'
== docs/howto/custom-template-tags.txt ==
> Filters and time zones
I think this section needs a 'versionadded' directive.
> to to your filter
^ typo
== docs/ref/models/querysets.txt ==
> When :doc:`time zone support </topics/i18n/timezones>` is enabled, the
> database stores datetimes in UTC, which means the aggregation is
> performed in UTC. This is a known limitation of the current
> implementation.
This implies that when time zone support is not enabled, datetimes are
not stored in UTC - which is false at least for Postgres. Perhaps this
could be reworded in some way?
Similarly with the next warning about 'year', 'month' etc. lookups.
> - 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.
== docs/ref/utils.txt ==
Needs a versionadded directive for django.utils.dateparse and
django.utils.timezone
== docs/topics/i18n/timezones.txt ==
> Provided the current time zone raises an exception for datetimes that
> don't exist or are ambiguous because they fall in a DST transition
> (pytz_ does), such datestimes will be reported as invalid values.
There is a typo - datestimes - but I found the paragraph confusing due
it starting with 'Providing'. Perhaps this would be better:
> If the current time zone raises an exception for datetimes that
> don't exist or are ambiguous because they fall in a DST transition
> (the timezones provided by pytz_ do this), such datetimes will be
> reported as invalid values.
== 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
> During this project, most things went according to the initial plan.
>
> There is only one outstanding issue that I know of.
> `QuerySet.dates()` operates in the database timezone, ie. UTC, while
> an end user would expect it to take into account its current time
> zone. This affects `date_hierarchy` in the admin and the date based
> generic views. The same problem also exists for the `year`, `month`,
> `day`, and `week_day` lookups.
>
> Under PostgreSQL, provided the current time zone has a valid name
> (e.g. it's provided by pytz), we could temporarily set the
> connection's timezone to this time zone and restore it to UTC
> afterwards. With other backends, I can't think of a simpler solution
> than fetching all values and doing the aggregation in Python. Neither
> of these thrills me, so for the time being, I've documented this as
> a known limitation.
You could argue a case for saying that the behaviour has some
advantages: if one admin user filters a list of objects by date, and
sends the link to a colleague, they would probably expect the colleague
to end up with the same list of objects.
But even if that isn't very convincing, I'm happy with this limitation
given the implementation difficulties of fixing it.
Great work, BTW.
Luke
--
"Trouble: Luck can't last a lifetime, unless you die young."
(despair.com)
Luke Plant || http://lukeplant.me.uk/
--
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.
