Message109061
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. |
|
Date |
User |
Action |
Args |
2010-07-01 16:42:40 | belopolsky | set | recipients:
+ 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:40 | belopolsky | set | messageid: <1278002560.76.0.579280516538.issue7989@psf.upfronthosting.co.za> |
2010-07-01 16:42:39 | belopolsky | link | issue7989 messages |
2010-07-01 16:42:37 | belopolsky | create | |
|