issue5288: tzinfo objects with sub-minute offsets are not supported

[Benjamin, 2010-07-07 22:04:33]

It looks good except for a few nits and a lack of tests.

http://codereview.appspot.com/1698050/diff/1/2
File Modules/datetimemodule.c (right):

http://codereview.appspot.com/1698050/diff/1/2#newcode91
Modules/datetimemodule.c:91: #define HASTZINFO(p) (((_PyDateTime_BaseTZInfo
*)(p))->hastzinfo)
This changes the formatting convention of all above macros.

http://codereview.appspot.com/1698050/diff/1/2#newcode878
Modules/datetimemodule.c:878: if (tzinfo == Py_None)
This could go above the PyTZInfo_Check assert.

http://codereview.appspot.com/1698050/diff/1/2#newcode880
Modules/datetimemodule.c:880: offset = PyObject_CallMethod(tzinfo, name, "O",
tzinfoarg);
You can just use PyObject_CallMethodObjArgs.

http://codereview.appspot.com/1698050/diff/1/2#newcode957
Modules/datetimemodule.c:957: result = PyObject_CallMethod(tzinfo, "tzname",
"O", tzinfoarg);
Use PyObject_CallMethodObjArgs.

http://codereview.appspot.com/1698050/diff/1/2#newcode1061
Modules/datetimemodule.c:1061: else
When the if uses branches, so should else.

http://codereview.appspot.com/1698050/diff/1/2#newcode3020
Modules/datetimemodule.c:3020: if (! PyDateTime_Check(dt)) {
You could fix the formatting here.

http://codereview.appspot.com/1698050/diff/1/2#newcode3063
Modules/datetimemodule.c:3063: temp = result;
You could just as well declare "temp" here.

http://codereview.appspot.com/1698050/diff/1/2#newcode4789
Modules/datetimemodule.c:4789: result = PyObject_CallMethod(tzinfo, "fromutc",
"O", temp);
PyObject_CallMethodObjArgs.

[sasha, 2010-07-07 22:12:21]

On Wed, Jul 7, 2010 at 6:04 PM,  <musiccomposition@gmail.com> wrote:
> It looks good except for a few nits and a lack of tests.
>
Lack of tests?  Did you see the coverage files attached to the issue?
This patch is refactoring only - no change of behavior is intended.

[benjamin_python.org, 2010-07-07 22:13:22]

2010/7/7 Alexander Belopolsky <alexander.belopolsky@gmail.com>:
> On Wed, Jul 7, 2010 at 6:04 PM,  <musiccomposition@gmail.com> wrote:
>> It looks good except for a few nits and a lack of tests.
>>
> Lack of tests?  Did you see the coverage files attached to the issue?
> This patch is refactoring only - no change of behavior is intended.

Ah, I see. Go ahead. :)



-- 
Regards,
Benjamin

[sasha, 2010-07-07 22:29:43]

Thanks for the review.  I have a couple of questions about your comments. 
Please see below.

http://codereview.appspot.com/1698050/diff/1/2
File Modules/datetimemodule.c (right):

http://codereview.appspot.com/1698050/diff/1/2#newcode91
Modules/datetimemodule.c:91: #define HASTZINFO(p) (((_PyDateTime_BaseTZInfo
*)(p))->hastzinfo)
On 2010/07/07 22:04:34, Benjamin wrote:
> This changes the formatting convention of all above macros.

Do you mean the "(" alignment?  Line 86 already breaks that.  What is the best
practice here?  I seem to remember that PEP 7 discourages vertical alignment
like this.  I was actually planning to reduce whitespace to 1 in the definitions
above as a part of detabification cleanup.

http://codereview.appspot.com/1698050/diff/1/2#newcode878
Modules/datetimemodule.c:878: if (tzinfo == Py_None)
On 2010/07/07 22:04:34, Benjamin wrote:
> This could go above the PyTZInfo_Check assert.

Hmm.  Why would that matter?  I like precondition asserts to go before any
non-debug logic.

http://codereview.appspot.com/1698050/diff/1/2#newcode880
Modules/datetimemodule.c:880: offset = PyObject_CallMethod(tzinfo, name, "O",
tzinfoarg);
On 2010/07/07 22:04:34, Benjamin wrote:
> You can just use PyObject_CallMethodObjArgs.

Good catch.  I keep forgetting about that.

http://codereview.appspot.com/1698050/diff/1/2#newcode957
Modules/datetimemodule.c:957: result = PyObject_CallMethod(tzinfo, "tzname",
"O", tzinfoarg);
On 2010/07/07 22:04:34, Benjamin wrote:
> Use PyObject_CallMethodObjArgs.

dito

http://codereview.appspot.com/1698050/diff/1/2#newcode1061
Modules/datetimemodule.c:1061: else
On 2010/07/07 22:04:34, Benjamin wrote:
> When the if uses branches, so should else.

OK.  Will fix. I did not know that.

http://codereview.appspot.com/1698050/diff/1/2#newcode3020
Modules/datetimemodule.c:3020: if (! PyDateTime_Check(dt)) {
On 2010/07/07 22:04:34, Benjamin wrote:
> You could fix the formatting here.

No space after "!"?  OK.

http://codereview.appspot.com/1698050/diff/1/2#newcode3063
Modules/datetimemodule.c:3063: temp = result;
On 2010/07/07 22:04:34, Benjamin wrote:
> You could just as well declare "temp" here.

Right. Is

PyObject *temp = result;

acceptable or does it have to be on two lines?

http://codereview.appspot.com/1698050/diff/1/2#newcode4789
Modules/datetimemodule.c:4789: result = PyObject_CallMethod(tzinfo, "fromutc",
"O", temp);
On 2010/07/07 22:04:34, Benjamin wrote:
> PyObject_CallMethodObjArgs.

NB: review all PyObject_CallMethod calls.

[benjamin_python.org, 2010-07-07 22:34:05]

2010/7/7  <alexander.belopolsky@gmail.com>:
> Thanks for the review.  I have a couple of questions about your
> comments.  Please see below.
>
>
> http://codereview.appspot.com/1698050/diff/1/2
> File Modules/datetimemodule.c (right):
>
> http://codereview.appspot.com/1698050/diff/1/2#newcode91
> Modules/datetimemodule.c:91: #define HASTZINFO(p)
> (((_PyDateTime_BaseTZInfo *)(p))->hastzinfo)
> On 2010/07/07 22:04:34, Benjamin wrote:
>>
>> This changes the formatting convention of all above macros.
>
> Do you mean the "(" alignment?  Line 86 already breaks that.  What is
> the best practice here?  I seem to remember that PEP 7 discourages
> vertical alignment like this.  I was actually planning to reduce
> whitespace to 1 in the definitions above as a part of detabification
> cleanup.

Okay. I just wondered if you were aware of that.

>
> http://codereview.appspot.com/1698050/diff/1/2#newcode878
> Modules/datetimemodule.c:878: if (tzinfo == Py_None)
> On 2010/07/07 22:04:34, Benjamin wrote:
>>
>> This could go above the PyTZInfo_Check assert.
>
> Hmm.  Why would that matter?  I like precondition asserts to go before
> any non-debug logic.

It doesn't matter. I just thought it would be clearer. Keep it if you want.

>>
>> You could fix the formatting here.
>
> No space after "!"?  OK.

Yep.

>
> Right. Is
>
> PyObject *temp = result;
>
> acceptable or does it have to be on two lines?

That's fine.



-- 
Regards,
Benjamin

[sasha, 2010-07-07 23:16:45]

http://codereview.appspot.com/1698050/diff/1/2
File Modules/datetimemodule.c (right):

http://codereview.appspot.com/1698050/diff/1/2#newcode880
Modules/datetimemodule.c:880: offset = PyObject_CallMethod(tzinfo, name, "O",
tzinfoarg);
On 2010/07/07 22:04:34, Benjamin wrote:
> You can just use PyObject_CallMethodObjArgs.

No, I cannot.   PyObject_CallMethodObjArgs needs a PyObject * as a method name
and I have a char *.