[Python-Dev] [Python-checkins] r88197 - python/branches/py3k/Lib/email/generator.py
Georg Brandl
g.brandl at gmx.net
Wed Jan 26 19:08:36 CET 2011
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
More information about the Python-Dev
mailing list