Antoine Pitrou <pit...@free.fr> added the comment: > > - "assertEqual = self.assertEqual" is more of a nuisance than anything > > else; > >you're making the code more complicated to read just to save a few > >keystrokes; > >same for "assertIsInstance = self.assertIsInstance" > > I'm not sure how it's more complicated or harder to understand. I didn't do > it > just to save keystrokes when writing, it's also to avoid noise when reading. > IMO > this is a question of personal taste, or is this approach proscribed > somewhere?
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. > > - in r_string(), what happens if PyBytes_Check(data) is false? there > > should > >probably be a TypeError of some sort > > There is a PyBytes_Check in marshal_load which raises a TypeError if it > fails. > The PyBytes_Check in r_string is only called in the path from marshal_load > (because p->readable is only non-NULL in that path) - so for the failure to > occur one would need an initial read to deliver bytes, and a subsequent read > to > deliver non-bytes. Still, I have no problem adding a TypeError raising in > r_string if PyBytes_Check fails. 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. > > - why do you call read(1) at the beginning? it seems to make the code more > >complicated for no useful purpose > > I gave the reasoning in the comment just above the read(1). I agree it makes > it > a little more complicated, but not onerously so - and the approach fails fast > as > well as allowing operation with any stream, not just random-seekable ones. 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. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue12291> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
