Message107737
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. |
|
Date |
User |
Action |
Args |
2010-06-13 15:53:52 | belopolsky | set | recipients:
+ 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:52 | belopolsky | set | messageid: <1276444432.59.0.0662202055155.issue5094@psf.upfronthosting.co.za> |
2010-06-13 15:53:51 | belopolsky | link | issue5094 messages |
2010-06-13 15:53:50 | belopolsky | create | |
|