This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author belopolsky
Recipients amaury.forgeotdarc, belopolsky, brett.cannon, brian.curtin, daniel.urban, lemburg, mark.dickinson, pitrou, r.david.murray, rhettinger, techtonik, tim.peters, vstinner
Date 2010-07-01.16:42:37
SpamBayes Score 1.4692338e-05
Marked as misclassified No
Message-id <1278002560.76.0.579280516538.issue7989@psf.upfronthosting.co.za>
In-reply-to
Content
Thanks a lot for the review.  Please see my replies below.

On Thu, Jul 1, 2010 at 12:09 PM, Antoine Pitrou <report@bugs.python.org> wrote:
..
> - I find the _cmp() and __cmp() indirection poor style in 3.x,
> especially when you simply end up comparing self._getstate() and
> other._getstate() (it is also suboptimal because it can do more
> comparisons than needed)
>

I agree.  Do you think I should just define __lt__ and use functools.total_ordering decorator?  Note that current implementation mimics what is done in C, but I think python should drive what is done in C and not the other way around. 

> - Shouldn't __eq__ and friends return NotImplemented if the other type
> mismatches, to give the other class a chance to implement its own
> comparison method? that's what built-in types do, as least
> (this would also make _cmperror useless)

This is a tricky part.  See issue #5516.  I would rather not touch it unless we want to revisit the whole comparison design.

>
> - Using assert to check arguments is bad. Either there's a risk of bad > input, and you should raise a proper error (for example ValueError),
> or there is none and the assert can be left out.
>

I disagree.  Asserts as executable documentation are good.  I know, -O is disfavored in python, but still you can use it to disable asserts.  Also I believe most of the asserts are the same in C version.

> - Starting _DAYS_IN_MONTH with a None element and then iterating over
> _DAYS_IN_MONTH[1:] looks quirky
>
Would you rather start with 0 and iterate over the whole list?  It may be better to just define it as a literal list display.  That's what C code does.

> - Using double-underscored names such as __day is generally
> discouraged, simple-underscored names (e.g. _day) should be preferred
>

I think in this case double-underscored names are justified.  Pickle/cPickle experience shows that people tend to abuse the freedom that python implementations give to subclasses and then complain that C version does not work for them.  I think __ name mangling will be a better deterrent than _ is private convention.

> - Some comments about "CPython compatibility" should be removed
>

Why?  The goal is to keep datetime.py in sync with datetimemodule.c, not to replace the C implementation.  C implementation will still be definitive.


> - Some other comments should be reconsidered or removed, such as
> "# XXX The following should only be used as keyword args"

This one I was actually thinking about making mandatory by changing the signature to use keyword only arguments.  I am not sure if that is well supported by C API, though.  

> or "XXX Buggy in 2.2.2"

Yes, a review of XXXs is in order.

>
> - Some things are inconsistent: date uses bytes for pickle support,
> time uses str for the same purpose

Already fixed.
History
Date User Action Args
2010-07-01 16:42:40belopolskysetrecipients: + belopolsky, lemburg, tim.peters, brett.cannon, rhettinger, amaury.forgeotdarc, mark.dickinson, pitrou, vstinner, techtonik, r.david.murray, brian.curtin, daniel.urban
2010-07-01 16:42:40belopolskysetmessageid: <1278002560.76.0.579280516538.issue7989@psf.upfronthosting.co.za>
2010-07-01 16:42:39belopolskylinkissue7989 messages
2010-07-01 16:42:37belopolskycreate