Message92069
| Author |
mark.dickinson |
| Recipients |
alexandre.vassalotti, bob.ippolito, ede, josiahcarlson, loewis, mark.dickinson, mwh, pitrou, rhettinger, tim.peters |
| Date |
2009年08月29日.20:46:40 |
| SpamBayes Score |
0.0 |
| Marked as misclassified |
No |
| Message-id |
<1251578804.36.0.535744071928.issue1023290@psf.upfronthosting.co.za> |
| In-reply-to |
| Content |
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? |
|