On Wed, Jan 26, 2011 at 7:57 PM, Victor Stinner wrote:

I was stupid to not run at least test_email, sorry. And no, I didn't ask for a review, because I thought that such minor change cannot be harmful.

During the RC period, *everything* that touches the code base should be reviewed by a second committer before checkin, and sanctioned by the RM as well. This applies even for apparently trivial changes. Docs checkins are slightly less strict (especially Raymond finishing off the What's New), but even there it's preferable to be cautious in the run up to a final release. Cheers, Nick. -- Nick Coghlan   |   ncoghlan@gmail.com   |   Brisbane, Australia

Am 26.01.2011 10:57, schrieb Victor Stinner:

Hi,

Le mardi 25 janvier 2011 à 18:07 -0800, Brett Cannon a écrit :

...

This broke the buildbots (R. David Murray thinks you may have forgotten to call super() in the 'payload is None' branch). Are you getting code reviews and fully running the test suite before committing? We are in RC. (...)

...

- if _has_surrogates(msg._payload): - self.write(msg._payload) + payload = msg.get_payload() + if payload is None: + return + if _has_surrogates(payload): + self.write(payload)

I didn't realize that such minor change can do anything harmful:

That's why the rule is that *every change needs to be reviewed*, not *every change that doesn't look harmful needs to be reviewed*. (This is true only for code changes, of course. Doc changes rarely have hidden bugs, nor are they embarrassing when a bug slips into the release. And I get the "test suite" (building the docs) results twice a day and can fix problems myself.)

the parent method (Generator._handle_text) has exaclty the same test. If msg._payload is None, call the parent method with None does nothing. But _has_surrogates() doesn't support None.

The problem is not the test of None, but replacing msg._payload by msg.get_payload(). I thought that get_payload() was a dummy getter reading self._payload, but I was completly wrong :-)

I was stupid to not run at least test_email, sorry. And no, I didn't ask for a review, because I thought that such minor change cannot be harmful.

I hope you know better now :) *Always* run the test suite *before* even asking for review. Georg