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?
History
Date User Action Args
2009-08-29 20:46:45mark.dickinsonsetrecipients: + mark.dickinson, mwh, tim.peters, loewis, rhettinger, josiahcarlson, bob.ippolito, pitrou, alexandre.vassalotti, ede
2009-08-29 20:46:44mark.dickinsonsetmessageid: <1251578804.36.0.535744071928.issue1023290@psf.upfronthosting.co.za>
2009-08-29 20:46:42mark.dickinsonlinkissue1023290 messages
2009-08-29 20:46:40mark.dickinsoncreate