Author mark.dickinson
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.14:09:06
SpamBayes Score 0.000725331
Marked as misclassified No
Message-id <1276438148.67.0.549637021497.issue5094@psf.upfronthosting.co.za>
In-reply-to
Content
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?
History
Date User Action Args
2010-06-13 14:09:09mark.dickinsonsetrecipients: + mark.dickinson, tim.peters, doerwalter, brett.cannon, belopolsky, ggenellina, pitrou, techtonik, ajaksu2, kawai, eric.araujo, r.david.murray, rafe, daniel.urban, l0nwlf, akira
2010-06-13 14:09:08mark.dickinsonsetmessageid: <1276438148.67.0.549637021497.issue5094@psf.upfronthosting.co.za>
2010-06-13 14:09:07mark.dickinsonlinkissue5094 messages
2010-06-13 14:09:06mark.dickinsoncreate