Mark Dickinson <dicki...@gmail.com> added the comment:
The patch looks great! Some comments:
- I think the type check for length_obj in long_tobytes should be more
lenient: I'd suggest a PyIndex_Check and PyNumber_AsSsize_t conversion
instead of the PyLong_Check. Or just use 'n' instead of 'O' in the
PyArg_Parse* format; this uses PyNumber_Index + PyLong_AsSsize_t, which
amounts to the same thing (or at least I *think* it does).
- I like the pickle changes, but I think they should be committed
separately. (Unless they're somehow required for the rest of the patch
to function correctly?)
- Stylistic nit: There's some inconsistency in the NULL checks in the
patch: "if (args != NULL)" versus "if (is_signed_obj)". PEP 7 doesn't
say anything about this, but the prevailing style in this file is for an
explicit '== NULL' or '!= NULL'.
- I'm getting one failing test:
test.support.TestFailed: Traceback (most recent call last):
File "Lib/test/test_long.py", line 1285, in test_frombytes
self.assertRaises(TypeError, int.frombytes, "", 'big')
AssertionError: TypeError not raised by frombytes
This obviously has to do with issue 6687; as mentioned in that issue,
I'm not sure that this should be an error. How about just removing this
test for now, pending a decision on that issue?
- Nice docs (and docstrings)!
On the subject of backporting to 2.7, I haven't seen any objections, so
I'd say we should go for it. (One argument for not backporting new
features is to provide incentive for people to upgrade, but I can't
realistically see this addition as a significant 'carrot'.) I'm happy
to do the backport, and equally happy to leave it to Alexandre if he's
interested.
Leaving the bikeshedding until last:
Method names: I'm +0 on adding the extra underscore. Python's already a
bit inconsistent (e.g., float.fromhex and float.hex). Also, at the time
the float.fromhex and float.hex names were being discussed, 'hex' seemed
to be preferred over 'tohex'; I wonder whether we should have int.bytes
instead of int.to_bytes?
----------
_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue1023290>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
