> Antoine Pitrou <pitrou@free.fr> added the  comment:

> It's not proscribed, but trying to remove the "self." because  it's
> supposed to be more readable is a bit of a strange thing to do.
> Also,  people reading the test suite should be accustomed to
> "self.assertEqual"  anyway, so there's no point trying to hide it.

It wasn't particularly about self - I'm not against it. Anyway, it's not a big 
deal for me, so I've added the selves back :-)

> Error checking can't  just be probabilistic. Perhaps there's a bug in the
> file-like object; or  perhaps it is a non-blocking IO object and read()
> will return None at  times.

You're right, so I've raised a TypeError if PyBytes_Check fails in r_string.

> Well, it  wouldn't fail any slower if you didn't do it, since you need to
> call read()  very soon anyway (presumably as part of the same call to
> marshal.load()).  Failing "fast" doesn't seem to bring anything here. My
> vote is for removing  the  complication.

Actually I misremembered the complete reason for the call - it was there to 
additionally check that the passed object has a read method. I also realised - 
duh - that I can read zero bytes and still get an empty bytes object back, so 
I've done that, and it does look cleaner. I've also reorganised the marshal_load 
function a little so it flows better.

Your other points have also all been addressed:

I've seen that the original test with the long binary data was exercising the 
same functionality as Engelbert Gruber's later patch was testing. So I've 
removed my earlier test entirely and added tuples and lists into the data being 
marshalled in Engelbert's version of the test. As a result of these changes, the 
zlib/base64 dependency goes away.

The new "readable" field of the C struct has a comment saying what it is.

The commented out r_byte macro has been removed.

Thanks, again, for the review!