Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

datetime lacks concrete tzinfo implementation for UTC #49344

Closed
brettcannon opened this issue Jan 29, 2009 · 91 comments
Closed

datetime lacks concrete tzinfo implementation for UTC #49344

brettcannon opened this issue Jan 29, 2009 · 91 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@brettcannon
Copy link
Member

BPO 5094
Nosy @tim-one, @doerwalter, @brettcannon, @mdickinson, @abalkin, @pitrou, @devdanzin, @ezio-melotti, @merwok, @bitdancer, @durban, @4kir4
Files
  • next-patch.txt: against 2.7
  • issue5094.diff
  • issue5094a.diff
  • localtime.py: aware local time implementation
  • datetimeex.py: Python prototype for 3-argument timezone
  • issue5094b.diff
  • issue5094c.diff
  • issue5094d.diff
  • issue5094d1.diff: Fixed utcnow() documentation. No code change from issue5094d.diff.
  • issue5094e.diff
  • issue5094f.diff: 'UTC±HH:MM' and more unit tests
  • issue5094g.diff
  • issue5094h.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/abalkin'
    closed_at = <Date 2010-06-16.02:02:27.617>
    created_at = <Date 2009-01-29.00:52:42.326>
    labels = ['extension-modules', 'type-feature']
    title = 'datetime lacks concrete tzinfo implementation for UTC'
    updated_at = <Date 2010-07-10.21:35:17.148>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2010-07-10.21:35:17.148>
    actor = 'eric.araujo'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2010-06-16.02:02:27.617>
    closer = 'belopolsky'
    components = ['Extension Modules']
    creation = <Date 2009-01-29.00:52:42.326>
    creator = 'brett.cannon'
    dependencies = []
    files = ['17455', '17525', '17533', '17541', '17544', '17569', '17570', '17585', '17586', '17631', '17648', '17657', '17680']
    hgrepos = []
    issue_num = 5094
    keywords = ['patch']
    message_count = 91.0
    messages = ['80735', '80740', '80745', '80807', '80857', '80866', '81043', '81086', '81119', '81641', '81649', '82818', '106403', '106411', '106412', '106413', '106414', '106415', '106422', '106445', '106476', '106478', '106483', '106484', '106485', '106487', '106490', '106493', '106494', '106496', '106498', '106518', '106565', '106911', '106914', '106920', '106923', '106971', '106973', '106974', '106976', '106977', '106980', '106997', '106998', '107006', '107008', '107059', '107060', '107072', '107092', '107105', '107107', '107158', '107186', '107189', '107190', '107212', '107279', '107290', '107291', '107545', '107546', '107547', '107548', '107552', '107554', '107569', '107608', '107628', '107668', '107670', '107676', '107683', '107733', '107737', '107738', '107742', '107743', '107786', '107787', '107819', '107874', '107886', '107889', '107890', '107891', '107892', '107893', '107894', '107901']
    nosy_count = 17.0
    nosy_names = ['tim.peters', 'doerwalter', 'brett.cannon', 'mark.dickinson', 'belopolsky', 'ggenellina', 'pitrou', 'techtonik', 'ajaksu2', 'kawai', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'rafe', 'daniel.urban', 'l0nwlf', 'akira']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue5094'
    versions = ['Python 3.2']

    @brettcannon
    Copy link
    Member Author

    When you call datetime.datetime.utcnow() you get back a naive datetime
    object. But why? You asked for UTC as the timezone based on what method
    call you made. And UTC is a very concrete timezone that never changes.

    It would be nice to have a concrete UTC tzinfo class that utcnow() uses
    so that at least those datetime instances are non-naive.

    If people have no issues with making this happen I will write the code
    for the concrete UTC tzinfo instance and make the appropriate changes to
    utcnow().

    @brettcannon brettcannon added the extension-modules C modules in the Modules dir label Jan 29, 2009
    @devdanzin
    Copy link
    Mannequin

    devdanzin mannequin commented Jan 29, 2009

    Brett,
    It might be worth to update tzinfo-examples.py to use your concrete UTC
    then:

    http://svn.python.org/view/python/trunk/Doc/includes/tzinfo-examples.py?rev=62214&view=markup

    @brettcannon
    Copy link
    Member Author

    On Wed, Jan 28, 2009 at 18:17, Daniel Diniz <report@bugs.python.org> wrote:

    Daniel Diniz <ajaksu@gmail.com> added the comment:

    Brett,
    It might be worth to update tzinfo-examples.py to use your concrete UTC
    then:

    I will if people are generally okay with the idea of adding this class.

    @kawai
    Copy link
    Mannequin

    kawai mannequin commented Jan 30, 2009

    I want UTC tzinfo. too.

    @ggenellina
    Copy link
    Mannequin

    ggenellina mannequin commented Jan 31, 2009

    Seems perfectly reasonable to me.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 31, 2009

    Please do. The current situation where the doc tells you not to use
    "naive" datetime objects but Python gives you no way to do so is awful.

    @brettcannon brettcannon added the type-feature A feature request or enhancement label Feb 3, 2009
    @vstinner
    Copy link
    Member

    vstinner commented Feb 3, 2009

    The UTC class have to be converted to C. Can someone write a patch for
    datetimemodule.c (and the doc plus an unit test ;-))?

    @brettcannon
    Copy link
    Member Author

    On Tue, Feb 3, 2009 at 03:28, STINNER Victor <report@bugs.python.org> wrote:

    STINNER Victor <victor.stinner@haypocalc.com> added the comment:

    The UTC class have to be converted to C.

    Yes, the example code is just an example. =)

    Can someone write a patch for
    datetimemodule.c (and the doc plus an unit test ;-))?

    I might have some people lined up to take this on.

    @brettcannon brettcannon self-assigned this Feb 3, 2009
    @rafe
    Copy link
    Mannequin

    rafe mannequin commented Feb 4, 2009

    I'm going to attempt to implement this feature.

    @doerwalter
    Copy link
    Contributor

    The patch doesn't include any changes to the documentation.

    @rafe
    Copy link
    Mannequin

    rafe mannequin commented Feb 11, 2009

    I thought I had uploaded this last night, apologies.

    @brettcannon
    Copy link
    Member Author

    I am currently doing a review of the patch over at
    http://codereview.appspot.com/22042 .

    @brettcannon brettcannon removed their assignment Nov 3, 2009
    @brettcannon
    Copy link
    Member Author

    Attaching a new patch by Rafe against Python 2.7. Unfortunately with 2.7 striving for an RC next, this should only target Python 3.2 and not 2.7.

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    I have two questions about the proposed implementation:

    1. Why not follow pytz lead and expose an instance of UTC rather than the UTC class itself?

    2. Is there a real need to add a boolean argument to utcnow()? I think timedelta.now(UTC()) or with utc = UTC() timedelta.now(utc) seems to be a more obvious way to produce TZ aware datetime.

    If a singleton instance utc is exposed instead of UTC class, I would suggest to change its repr to 'datetime.utc'.

    On the patch itself, datetime_utcnow() is missing an error check for PyObject_IsTrue() return value:

    >>> class X:
    ...    def __nonzero__(self): raise RuntimeError
    ... 
    >>> datetime.utcnow(tz_aware=X())
    datetime.datetime(2010, 5, 25, 2, 12, 14, 739720, tzinfo=<datetime.UTC object at 0x1015aab80>)
    XXX undetected error
    ..

    @rafe
    Copy link
    Mannequin

    rafe mannequin commented May 25, 2010

    Alexander, about 1, that's a pretty good question. I had originally wanted to do something like that but Brett Cannon at the time thought it was not the right approach. I don't recall the details. Maybe Brett can recall. I think we had that conversation in person and it was a long time ago :(

    I had originally thought of doing the class, and then having constants associated with it:

    UTC.UTC0

    Eventually if we support multiple timezones:

    UTC.UTC1
    UTC.UTC2
    UTC.UTC_1
    UTC.UTC_2

    Well... maybe not given how impossible the naming would be.

    I think we also talked about redefining new so that it would always return the same global instance.

    On 2, we had discussions about how to pass parameters in to utcnow that we DID record. When I suggested it, Brett said:

    "...using a boolean flag over an argument is much less error-prone for a developer from passing in the wrong timezone object; passing in something other than an instance of UTC would just be stupid so we should make sure the developer isn't stupid. =)"

    @brettcannon
    Copy link
    Member Author

    We didn't do a singleton because I don't like singletons. =) Plus they muck with isinstance/issubclass if you don't expose the class. Basically there is no need to have it be a singleton, so why bother?

    And Rafe is right: since utcnow() already exists, why not take advantage of the method? Yes, you could manually call now() with a UTC object, but people are going to notice the utcnow() method and want to use it, so we should make it easy to use the new UTC object on utcnow(). Plus it has the added benefit of providing a transition plan to make utcnow() always return a timezone-aware datetime object.

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    On Mon, May 24, 2010 at 11:06 PM, Rafe Kaplan <report@bugs.python.org> wrote:
    ..

    On 2, we had discussions about how to pass parameters in to utcnow that we DID record.  When I
    suggested it, Brett said:

     "...using a boolean flag over an argument is much less error-prone for a developer from passing in the wrong
    timezone object; passing in something other than an instance of UTC would just be stupid so we should make
    sure the developer isn't stupid. =)"

    Well, I respectfully disagree. This advise seems to be placing
    convenience of the writer of the code over that of the reader.
    Imagine encountering an expression datetime.utcnow(True) allowed by
    your current patch and trying to figure out what it means. This can
    be improved by making tz_aware a keyword only argument, but in that
    case a separate datetime.tz_aware_utcnow() function seems like a
    better choice.

    Note that I am not suggesting passing anything to utcnow(). I would
    either leave it unchanged or make it always return aware datetime
    instances. (Note that with singleton UTC timezone naive datetime
    instances can be deprecated with no performance penalty.)

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    We didn't do a singleton because I don't like singletons. =) Plus they
    muck with isinstance/issubclass if you don't expose the class.

    I am not sure what you mean by "muck with." Why would anyone want to subclass UTC?

    Basically there is no need to have it be a singleton, so why bother?

    There are several advantages of having all datetime instances with tzinfo=UTC() referring to the same instance:

    1. Comparison (and I believe subtraction) of aware datetime instances bypasses calculation of utcoffset if their tzinfo attributes refer to the same object.

    2. With the current patch,

    >>> set(UTC() for i in range(3))
    set([<datetime.UTC object at 0x1015aac80>, <datetime.UTC object at 0x1015aad00>, <datetime.UTC object at 0x101a0e040>])

    I don't think this is intended. Basically UTC() instances cannot be meaningfully compared or used as dictionary or set keys. You can fix it by providing custom __eq__ and __hash__, but this problem simply goes away if a singleton is exposed instead.

    1. now(utc) is slightly more readable than now(UTC())

    2. Singleton utc is familiar to pytz users.

    @pitrou
    Copy link
    Member

    pitrou commented May 25, 2010

    Note that I am not suggesting passing anything to utcnow(). I would
    either leave it unchanged or make it always return aware datetime
    instances.

    The latter would break compatibility, though (especially given how
    comparison between "naive" and "aware" datetimes raises an error...).

    I also agree with Brett that a singleton looks rather unnecessary (it
    also look quite C++/Java-esque to me).

    On the subject of the patch:

    • Alexander spotted the PyObject_IsTrue() issue
    • you should never use tabs (anymore), only 4 spaces
    • lines like:
      self.assertTrue(isinstance(now.tzinfo, UTC))
      can be rewritten as
      self.assertIsInstance(now.tzinfo, UTC)
    • here, I'm not sure why you're using "UTC" as the error message
      (copy/paste error?):
      self.assertEquals(None, meth(False).tzinfo, UTC)
    • you might make the UTC class subclassable, although I'm not sure
      there's any point

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    On Tue, May 25, 2010 at 5:45 AM, Antoine Pitrou <report@bugs.python.org> wrote:
    ..

    I also agree with Brett that a singleton looks rather unnecessary (it
    also look quite C++/Java-esque to me).

    I still don't understand your aversion to singletons and you did not
    address any of the advantages that I listed in my previous comment. I
    don't think singletons are foreign to Python: after all we write None
    rather than NoneType() .

    We can reach a middle ground by interning UTC instances behind the
    scenes so that UTC() is UTC() will always be true. This will address
    most of the issues that I raised and utc = datetime.UTC() is simple
    enough to write as long as you don't have to worry about sharing utc
    instance between modules.

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    On Mon, May 24, 2010 at 11:06 PM, Rafe Kaplan <report@bugs.python.org> wrote:
    ..

    I had originally thought of doing the class, and then having constants associated with it:

     UTC.UTC0

    Eventually if we support multiple timezones:

     UTC.UTC1
    ..

    Well... maybe not given how impossible the naming would be.
    ..

    I actually like your original idea. It seems wasteful to create a
    concrete timezone class in a C module and only use it for a single
    timezone. FixedOffset class in tzinfo-examples.py is only slightly
    more complicated than UTC class and as explained in the comment above
    it, "FixedOffset(0, "UTC") is a different way to build a UTC tzinfo
    object. FixedOffset objects can then be used to produce aware
    datetime instances from strptime. (See bpo-6641.) I would only
    define utc = FixedOffset(0, "UTC") instance and make name argument to
    FixedOffset optional defaulting to UTC(+/-)hhmm.

    @brettcannon
    Copy link
    Member Author

    The singleton dislike from Antoine and me is that they are generally just not liked in the stdlib. None/True/False are special cases because they are syntax, so having None is None ever not work would just be weird. Otherwise singletons are unnecessary in Python. Just look through the stdlib and you will find very few singletons as they are generally considered bad. Having to write a custom eq or hash is just part of being explicit. And trying to make a factory function that always returns the same instance is not a solution either. I understand pytz might use them, but this is the stdlib, so we need to go with what we consider best practice for Python since it will lead to much more use than pytz gets.

    Now if a simple FixedOffsetTimeZone class was added and we just pre-populated the datetime module with a utc attribute that contained an instance of that class set to the proper values for UTC, that I could support without controversy. That would get you your "singleton" by reliably using the same instance without having to try to hack in singleton support.

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    ..
    Thanks for the explanation. I realize that I should not have used the
    s-word. :-) In fact I only wanted a module level constant utc = UTC()
    and did not care much about other UTC class instances and whether any
    are permitted or easy to create.

    Well, the datetime module is not exactly the place you want to start
    if you want to lead anyone to best Python practices. :-) (Just think
    of datetime subclassing from date!)

    Now if a simple FixedOffsetTimeZone class was added and we just pre-populated the datetime module with a utc attribute that contained an
    instance of that class set to the proper values for UTC, that I could support without controversy.

    This is exactly my preferred solution.

    @rafe
    Copy link
    Mannequin

    rafe mannequin commented May 25, 2010

    "Note that I am not suggesting passing anything to utcnow(). I would
    either leave it unchanged or make it always return aware datetime
    instances."

    FYI, all other issues aside, having utcnow() (with no parameters) return a tzaware instance will introduce backward compatibility problems. As it is, users are not expecting utcnow to return a date-time with any tzinfo.

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    Roundup bug bites again. Reposting via web:

    -----
    On Tue, May 25, 2010 at 5:35 PM, Brett Cannon <report@bugs.python.org> wrote:

    The singleton dislike from Antoine and me is that they are generally just not liked in the stdlib.
    ..
    Thanks for the explanation. I realize that I should not have used the
    s-word. :-) In fact I only wanted a module level constant utc = UTC()
    and did not care much about other UTC class instances and whether any
    are permitted or easy to create.

    .. so we need to go with what we consider best practice for Python since it will lead to much more use than pytz gets.

    Well, the datetime module is not exactly the place you want to start
    if you want to lead anyone to best Python practices. :-) (Just think
    of datetime subclassing from date!)

    Now if a simple FixedOffsetTimeZone class was added and we just pre-populated the datetime module with a utc attribute that contained an
    instance of that class set to the proper values for UTC, that I could support without controversy.

    This is exactly my preferred solution.

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    Note that Brett has already mentioned backward compatibility issues, but suggested that "[adding tz_aware argument may provide] a transition plan to make utcnow() always return a timezone-aware datetime object." [msg106413]

    I would say, lets leave utcnow() alone. It is ugly enough without a boolean argument. I don't see how datetime.now(utc) can be too error prone. I think Brett's comment about a stupid developer was about passing tzinfo instead of bool to utcnow() and that I agree makes no sense.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 11, 2010

    I am attaching a new patch, issue5094e.diff which addresses most of Mark's comments. I left out repr() because two opinions were voiced on IRC with respect to datetime. prefix. I would like to give it some more thought even though I am leaning towards compatibility with existing reprs.

    I did not make td argument optional and did not allow timezone o be subclassed because these seem to be mutually exclusive options. (If td is optional in base class, it must be optional in subclasses per Liskov's principle severely limiting utility of subclasses.) Let's address this separately.

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Jun 11, 2010

    Minor notes:

    msg107186:

    1. The constructor now accepts only whole number of minutes in [-23:59, 23:59] range.

    rfc 3339 provides the following example:

    1937-01-01T12:00:27.87+00:20

    This represents the same instant of time as noon, January 1, 1937,
    Netherlands time. Standard time in the Netherlands was exactly 19
    minutes and 32.13 seconds ahead of UTC by law from 1909-05-01 through
    1937-06-30. This time zone cannot be represented exactly using the
    HH:MM format, and this timestamp uses the closest representable UTC
    offset.

    The presence of fractions of seconds in time zone is an exception so
    it might not be worth to support it but it exists.

    msg107552:
    Similarly, should str(timezone.utc) be '+0000' or 'UTC' or '+00:00'?

    Excerpts in favor for '+00:00' from rfc 3339:

    Attempts to label local offsets with alphabetic
    strings have resulted in poor interoperability in the past [IMAIL],
    [HOST-REQ]. As a result, RFC2822 [IMAIL-UPDATE] has made numeric
    offsets mandatory.

    If the time in UTC is known, but the offset to local time is unknown,
    this can be represented with an offset of "-00:00". This differs
    semantically from an offset of "Z" or "+00:00", which imply that UTC
    is the preferred reference point for the specified time.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 12, 2010

    There is a separate issue bpo-5288 asking to support sub-minute offsets. This is not hard, but the C code still has a few interfaces left from the time when offset was an integer # of minutes. I am +1 to fix that, but not as a part of this issue.

    On str(tz), I definitely want an invariant str(tz) == tz.tzname(None). I am open to changes to tzname(), but we are very close to bikesheding here. Let's settle for 'UTC±HH:MM'. This seems to be the most common spelling for numeric timezones in various tables on the web.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 12, 2010

    I have made my mind on subclassing timezone issue. I believe subclassing should not be allowed and here is the reason:

    The new datetime.timezone class is a very specific implementation of tzinfo interface. It guarantees that utcoffset(dt) and friends return the same value for all times. Applications should be able to rely on this and a simple isinstance(tz, timezone) should be enough to deduce that tz is a fixed offset tzinfo instance. (Of course careful applications can check type(tz) == timezone instead, but making isinstance(tz, timezone) subtly wrong is not a good idea.)

    There is also very little to be gained from subclassing timezone. It defines only 4 methods and each is entirely trivial. Any non-trivial tzinfo implementation will need to override all 4 of them anyways. It is much safer to derive from tzinfo directly and possible reuse timezone through containment (for example to format offsets in a standard way.)

    The only immediate application of subclassing is to add dst() method returning a fixed offset rather than None. This is trivial to implement through containment and if such class becomes popular, I would prefer to add this functionality to timezone class itself.

    @mdickinson
    Copy link
    Member

    I agree it seems fine to disallow subclassing of timezone. (And if we decide, in the light of new information, that that was wrong, then it's easy to allow subclassing later; harder to disallow it.)

    +1 for 'UTC±HH:MM' for both tzname and __str__, too.

    I'll take a look at the newest patch.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 12, 2010

    +1 for 'UTC±HH:MM' for both tzname and __str__, too.

    It looks like I subconsciently allocated enough characters for this in the string buffer, so now this is a single character change. I'll do it with other changes (if any) before commit.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 12, 2010

    Attaching issue5094f.diff which implements 'UTC±HH:MM' and adds unit tests for more error cases.

    @mdickinson
    Copy link
    Member

    The latest patch looks good to me. I only have minor comments, in random order:

    • Should the PyDateTime_TimeZone struct definition go into datetime.h, so that it's avaiable if you want to export any C-API functions later on?

    • If you're not allowing subclassing, then presumably you don't need
      the new_timezone / new_timezone_ex dance?

    • For clarity, please consider adding parentheses in:

        self = (PyDateTime_TimeZone *)type->tp_alloc(type, 0);

    to make it:

        self = (PyDateTime_TimeZone *)(type->tp_alloc(type, 0));
    • Whitespace issues: there are a couple of tabs in the source (at around lines 810, 3388, 3390), and an overly long line (>79 characters) at around line 3365.

    • Please add a brief comment before the added C functions (like new_timezone_ex) explaining their purpose.

    • I wonder whether __ne__ should return the correct result (rather than returning NotImplemented) for timezone instances. I realize that it's not necessary to implement it in order for 'tz1 != tz2' to work, but it still makes me uncomfortable that it's not implemented. OTOH, I agree with the decision not to allow timezone order comparisons (though I bet they get requested at some point).

    • There doesn't seem to be any mention of timezone.min or timezone.max in the docs. Is this deliberate?

    @abalkin
    Copy link
    Member

    abalkin commented Jun 13, 2010

    On Sun, Jun 13, 2010 at 10:09 AM, Mark Dickinson <report@bugs.python.org> wrote:
    ..

    • Should the PyDateTime_TimeZone struct definition go into
      datetime.h, so that it's avaiable if you want to export any C-API
      functions later on?

    The original patch had this in the header file, but I moved it down to .c during dst debate. (See msg107186, point 3.) In the future we may add accessor functions to datetime.h, but this can be done without exposing the C struct.

    • If you're not allowing subclassing, then presumably you don't need
       the new_timezone / new_timezone_ex dance?

    I agree, but if you don't mind, I will not change it at this point. There is a small chance that there will be an outcry for supporting subclassing and we will put that back in. Also, issue bpo-2267 debate may result in removing the dance from other factory functions. (If the camp saying that the base class has no idea how to construct subclass instances wins.) Not a big deal either way, though.

    • For clarity, please consider adding parentheses in:

       self = (PyDateTime_TimeZone *)type->tp_alloc(type, 0);

    will do. PEP-7 does not mention that, but probably should.

    • Whitespace issues: there are a couple of tabs in the source (at
      around lines 810, 3388, 3390), and an overly long line (>79
      characters) at around line 3365.

    Thanks. I thought I checked those. Maybe I should take over bpo-8912 as a community service. :-)

    • Please add a brief comment before the added C functions (like
      new_timezone_ex) explaining their purpose.

    Will do.

    • I wonder whether __ne__ should return the correct result (rather
      than returning NotImplemented) for timezone instances.

    Will do.

     OTOH, I agree with the decision not to allow timezone order
    comparisons (though I bet they get requested at some point).

    I will support such request. It is very easy to implement (just remove the check that disallows it) and makes perfect sense. I left them out only because it is easier to add features than to remove in the future and I don't want to debate this now.

    • There doesn't seem to be any mention of timezone.min or timezone.max
      in the docs.  Is this deliberate?

    Yes. Without ordering, having min and max is rather strange. I wanted to expose these for testing and will definitely document them when (and if) order comparisons are implemented. For now, let's keep them as easter eggs.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 13, 2010

    Just to add a little bit of historical perspective, this proposal is really more than seven years old:

    """
    s.keim (Dec 20, 2002 3:33 am; Comment #13)

    .. do we really need methods like utcnow. I believe the following could be a little more easy to learn:

    Apparently, this proposal have been forgotten and reinvented again in msg106411, point 2.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 13, 2010

    issue5094g.diff addresses all Mark's suggestions except making struct definition public. I also made a few other changes:

    1. Constructor now raises TypeError when offset is not a timedelta instead of ValueError in previous version.

    2. NEWS entry no longer says that argument is ignored by tzinfo methods implemented in timezone. (Since these methods now check the type of the argument, it would be misleading to say that arg is ignored.)

    3. Added some more tests for error cases.

    4. Added ACK for Rafe Kaplan as the author of the original patch.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 13, 2010

    Also I changed Py_DECREFs in destructor to Py_CLEAR. I understand that while not strictly required in this case, it is a good practice.

    @mdickinson
    Copy link
    Member

    issue5904g.diff looks good to me. A very nice piece of work!

    @abalkin
    Copy link
    Member

    abalkin commented Jun 14, 2010

    Committed in r81981. I'll open a separate documentation issue to update Doc/includes/tzinfo-examples.py and improve TZ related documentation.

    @abalkin abalkin closed this as completed Jun 14, 2010
    @abalkin abalkin changed the title datetime lacks concrete tzinfo impl. for UTC datetime lacks concrete tzinfo implementation for UTC Jun 14, 2010
    @brettcannon
    Copy link
    Member Author

    Thanks for sticking with this, Alexander. I realized I was a slight pain to deal with on this one. =)

    @abalkin
    Copy link
    Member

    abalkin commented Jun 15, 2010

    Reopening to add some minor fixes to tests and documentation. See issue5094h.diff. Ezio, thanks for finding these issues.

    @abalkin abalkin reopened this Jun 15, 2010
    @merwok
    Copy link
    Member

    merwok commented Jun 15, 2010

    +1 for replacing math range notation with English. Much easier for non-math people that do Python :)

    One more nit: Your docstrings use verb forms like “Returns” where PEP 257 advises to use “Return”: “[the docstring] prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ..."”

    @abalkin
    Copy link
    Member

    abalkin commented Jun 15, 2010

    What about

    """
    .. method:: datetime.utcoffset()

    If :attr:`tzinfo` is ``None``, returns ``None``, else returns ...
    """

    Should this use "return" too?

    @merwok
    Copy link
    Member

    merwok commented Jun 15, 2010

    .. method:: datetime.utcoffset()

    Return ``None`` if :attr:`tzinfo` is ``None``, else else return ...

    That said, to keep diffs readable, perhaps stick with the existing
    convention know, and later a PEP-257 compliance diff can be made.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 15, 2010

    How to specify offset range horse got its beating above. See msg107554. The current wording is the best compromise between verbosity and precision.

    Replacing the patch with one that fixes other nits.

    @merwok
    Copy link
    Member

    merwok commented Jun 15, 2010

    Suggestion for one line:
    “The dt argument must be aware with tzinfo set to self.”
    “The dt argument must be an aware datetime, with tzinfo set to self.”

    If there is a reST construct for aware datetime, use it. Since “self” is not special, perhaps an alternate wording like “the timezone instance from which the method is called”, but less ugly.

    Some PyDoc_STR calls still use “Returns.”

    By the way, you could have committed the unittest changes directly.

    @techtonik
    Copy link
    Mannequin

    techtonik mannequin commented Jun 15, 2010

    Good job. Thanks for working on this.

    It is possible to backport this to future of Python 2.7?

    @pitrou
    Copy link
    Member

    pitrou commented Jun 15, 2010

    It is possible to backport this to future of Python 2.7?

    No.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 16, 2010

    On this happy note, I am closing this issue. Doc changes have been committed in r82004, test changes in r82003. For repr(timezone(..)) development, please follow issue bpo-9000.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants