Author belopolsky
Recipients ajaksu2, belopolsky, brett.cannon, daniel.urban, doerwalter, eric.araujo, ggenellina, kawai, l0nwlf, mark.dickinson, pitrou, r.david.murray, rafe, techtonik, tim.peters
Date 2010-06-11.15:23:41
SpamBayes Score 0.0
Marked as misclassified No
Message-id <1276269824.75.0.787420010269.issue5094@psf.upfronthosting.co.za>
In-reply-to
Content
I have to stop replying to emails.  There is no reason behind roundup remove ">" comments logic.   Reposting my message:

"""
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?

> - As noted above, the 'timezone' class can't be subclassed.  Is this deliberate?
>  I notice that Brett said "let users subclass as needed to add DST support" in msg107008.
>
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)]


> - The existing docs say, at one point: "if utcoffset does not return None,
> dst() should not return None either."  And yet it seems that this is exactly what happens for timezone instances: > utcoffset doesn't return None, but dst does.
>  Was there a reason for the explicit restriction in the docs, and are we sure that that reason is no longer valid?
>
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.


> - I find it strange that mytimezone.utcoffset(1+3j) works;  similarly
> for tzname and dst.  Perhaps it should be
> checked at least that the argument is a datetime.  Similarly for
> tzname and dst.
>
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 ignored.
>  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.

I can make the arg optional now.
>
> - Any chance of a nice __str__ implementation for timezone instances?
>  (And/or possibly a nice __repr__ as well)?
>
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'?

> - The docs for tzname are misleading:  they claim that the default name has the form "UTCsHHMM".  This isn't
> true for UTC+0, whose name seems to be just "UTC".  It actually wouldn't seem unreasonable to have this print
> as "UTC+0000", just for consistency (and for ease of parsing for anyone on the receiving end of such a string).
> Or the docs could be fixed.
>
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.

> - In the docs, replace "timezeone" with "timezone"
>
Good catch.  Thanks for the review.
History
Date User Action Args
2010-06-11 15:23:45belopolskysetrecipients: + belopolsky, tim.peters, doerwalter, brett.cannon, mark.dickinson, ggenellina, pitrou, techtonik, ajaksu2, kawai, eric.araujo, r.david.murray, rafe, daniel.urban, l0nwlf
2010-06-11 15:23:44belopolskysetmessageid: <1276269824.75.0.787420010269.issue5094@psf.upfronthosting.co.za>
2010-06-11 15:23:42belopolskylinkissue5094 messages
2010-06-11 15:23:42belopolskycreate