Re: [Python-Dev] [Python-checkins] r88197 - python/branches/py3k/Lib/email/generator.py
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. On Tue, Jan 25, 2011 at 16:39, victor.stinner <python-checkins@python.org> wrote:
Author: victor.stinner Date: Wed Jan 26 01:39:19 2011 New Revision: 88197
Log: Fix BytesGenerator._handle_text() if the message has no payload (None)
Modified: python/branches/py3k/Lib/email/generator.py
Modified: python/branches/py3k/Lib/email/generator.py ============================================================================== --- python/branches/py3k/Lib/email/generator.py (original) +++ python/branches/py3k/Lib/email/generator.py Wed Jan 26 01:39:19 2011 @@ -377,8 +377,11 @@ def _handle_text(self, msg): # If the string has surrogates the original source was bytes, so # just write it back out. - 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) else: super(BytesGenerator,self)._handle_text(msg)
_______________________________________________ Python-checkins mailing list Python-checkins@python.org http://mail.python.org/mailman/listinfo/python-checkins
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: 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. FYI the commit is related indirectly to #9124 (Mailbox module should use binary I/O, not text I/O). Victor
On Wed, Jan 26, 2011 at 7:57 PM, Victor Stinner <victor.stinner@haypocalc.com> 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
On Wed, Jan 26, 2011 at 04:34, Nick Coghlan <ncoghlan@gmail.com> wrote:
On Wed, Jan 26, 2011 at 7:57 PM, Victor Stinner <victor.stinner@haypocalc.com> 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.
Especially as this is not the first slip-up; Raymond had a copy-and-paste slip that broke the buildbots. Luckily he was in #python-dev when it happened and it was noticed fast enough he fixed in in under a minute. So yes, even stuff we would all consider minor **must** have a review. Time to update the devguide I think. -Brett
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
participants (4)
-
Brett Cannon -
Georg Brandl -
Nick Coghlan -
Victor Stinner