Author belopolsky
Recipients ajaksu2, akira, belopolsky, brett.cannon, daniel.urban, doerwalter, eric.araujo, ggenellina, kawai, l0nwlf, mark.dickinson, pitrou, r.david.murray, rafe, techtonik, tim.peters
Date 2010-06-13.15:53:50
SpamBayes Score 7.03907e-05
Marked as misclassified No
Message-id <1276444432.59.0.0662202055155.issue5094@psf.upfronthosting.co.za>
In-reply-to
Content
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 #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 issue8912 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.
History
Date User Action Args
2010-06-13 15:53:52belopolskysetrecipients: + belopolsky, tim.peters, doerwalter, brett.cannon, mark.dickinson, ggenellina, pitrou, techtonik, ajaksu2, kawai, eric.araujo, r.david.murray, rafe, daniel.urban, l0nwlf, akira
2010-06-13 15:53:52belopolskysetmessageid: <1276444432.59.0.0662202055155.issue5094@psf.upfronthosting.co.za>
2010-06-13 15:53:51belopolskylinkissue5094 messages
2010-06-13 15:53:50belopolskycreate