Author mark.dickinson
Recipients akitada, belopolsky, christian.heimes, josm, loewis, mark.dickinson, rhettinger, robertwb, zanella
Date 2010-05-02.08:37:38
SpamBayes Score 8.80617e-06
Marked as misclassified No
Message-id <1272789461.94.0.958146893344.issue1533@psf.upfronthosting.co.za>
In-reply-to
Content
Thanks---the new patch looks good.  Pulling the argument conversion out into a separate function makes the whole thing much cleaner.

I still have a couple of nits:

 - Please add a comment before get_range_argument indicating
   what it's for.  I'd also consider naming the function something
   more descriptive like 'convert_range_argument' rather than
   'get_range_argument', but I've never been good with names...

 - Good catch about checking the return type of nb_int.  The error
   message should refer to "__int__" though, not "nb_int":  "nb_int"
   won't make much sense to most Python users.

 - I notice that get_range_argument steals a reference to arg.  That's
   fine of course, but it's a little bit unusual, so there should be
   a comment pointing that out somewhere.  It *might* be preferable to
   not steal the reference, and just do the usual 'return a new
   reference' thing instead; I know that leads to a little bit
   more boilerplate in handle_range_longs, but I think the code ends
   up clearer, and there won't be surprises for someone who tries to
   reuse the code in get_range_argument for their own conversions.  I
   leave it up to you. :)

 - Please could you also add a test for small arguments for each test
   class?

 - Style nit:  the codebase mostly avoids assignments in 'if' conditions;
   please separate the assignment and the NULL test in lines like
   "if (!(ilow = get_range_argument(ilow, "start")))".  Also,
   "if (ilow == NULL)" is preferable to "if (!ilow)".

Thanks again for doing this.
History
Date User Action Args
2010-05-02 08:37:42mark.dickinsonsetrecipients: + mark.dickinson, loewis, rhettinger, belopolsky, christian.heimes, josm, robertwb, zanella, akitada
2010-05-02 08:37:41mark.dickinsonsetmessageid: <1272789461.94.0.958146893344.issue1533@psf.upfronthosting.co.za>
2010-05-02 08:37:40mark.dickinsonlinkissue1533 messages
2010-05-02 08:37:38mark.dickinsoncreate