[Python-Dev] [Python-checkins] r88197 - python/branches/py3k/Lib/email/generator.py

[Georg Brandl]

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