Mark Dickinson <dicki...@gmail.com> added the comment: 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. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue1533> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
