On Fri, Jun 11, 2010 at 10:23 AM, Mark Dickinson <report@bugs.python.org> wrote:
..
> Some comments from playing with this patch (without having looked at the implementation):
>
thank you very much for your comments.  As we are fine-tuning the
timezone class, do you think it would be helpful to go back to Brett's
suggestion and add datetime.py with just from _datetime import * and
keep parallel Python and C implementations of timezone class only
until full datetime.py is available?

I was deliberate in a sense that I thought about it, but there was no
substantial discussion.  Original patch did not allow subclassing of
the UTC class and I thought it was right.  (See msg106415 and
msg106422.) subclassing FixedOffsetTimeZone makes more sense, but I
was hoping to keep timezone complete and eventually stop supporting
tzinfo API (not allow passing datetime in the methods.)  More on this
below.  I would very much prefer not to open the subclassing debate.
If you support subclassing, I will add a flag. My only objection is
that it is much easier to add this functionality in the future than to
remove it if it gets abused.  Can we postpone this decision until real
life use case comes in?  (Subclass to change dst() in itself is not a
use case.  Why do you need legacy timetuple interface to work is the
first question.)

> - If I try to do  timezone(timedelta(hours=24)), I get an error message:
> "ValueError: offset must be a timedelta between timedelta(1) and -timedelta(1)." and I have to think for a bit to
> remember that 'timedelta(1)' means 'timedelta(days=1)'.  Any chance of making this more explicit:  e.g. "between > timedelta(hours=-24) and timedelta(hours=24)"?
>

Please make a call and I'll implement it.  The competing spellings are:

1. timedelta(hours=24)
2. 24 hours (I rejected this because it may be confused with an int)
3. timedelta(days=1)
4. in the range [-timedelta(hours=23, minutes=59), timedelta(hours=23,
minutes=59)]

Yes, there was a reason, but I don't think it is a good one.  The
default implementation of fromutc() does not work if utcoffset()
returns timedelta but dst() returns None.  The dst() value used to
detect DST ambiguities.  Remember, fromutc() despite the name is
defined to operate on local times and some local times are ambiguous
and some are invalid.  The default implementation attempts to detect
that.  This is the issue that I am trying to avoid in my design.

The test is easy to add.  My longer term plan is to disallow passing
argument to constant methods, so adding check is somewhat wasteful.
Will do.

> - And it also seems clunky that an argument *has* to be supplied for utcoffset, tzname and dst, only to be i
> gnored.  Would it be possible to make the argument optional?

Yes.  this was my transition plan.  Make it optional.  Modify datetime
methods to first try to get offset/name/dst without passing time
argument and fall back to passing self.  Remove optional argument from
timezone methods.

Yes.  I want parseable __repr__ as well.  I've been torn between
'datetime.timezone(datetime.timedelta(..)[, ..])'  and
'timezone(timedelta(..)[, ..])'. Opinions welcome.  Similarly, should
str(timezone.utc) be '+0000' or 'UTC' or '+00:00'?

You are reading my mind.  I planned to tear out conditional logic that
supports plain 'UTC', but apparently forgot.  Will do.

> - I'm very confused about utcoffset:  why can't I supply a UTC datetime (i.e. an aware datetime with
> tzinfo = timezone.utc) to this?  I suspect I'm misunderstanding something here...

You are not alone! :-)  My understanding is that utcoffset was
designed around the notion that if you add a day to 12 noon the day
before DST change, you should still get 12 noon the next day.
Supporting this requires deriving local timezone from local time which
is impossible for some datetime values.  Most business applications
can ignore this because nobody schedules meetings between 1 and 2 am
and those who do are smart enough to catch an exception and ask the
user to clarify what time he/she means.

Good catch.  Thanks for the review.