Issue #3602 - Code Review (original) (raw)

http://codereview.appspot.com/3255/diff/1/3#newcode52 Line 52: from sys import py3kwarning On 2008/08/23 01:46:36, Benjamin wrote:

I think it's better to just "import sys" here, so we don't have a bunch of modules with "py3kwarning" in them sitting around.

Maybe, but you could say this about any import. I don't see it as a problem.

http://codereview.appspot.com/3255/diff/1/10#newcode385 Line 385: return warnings.catch_warnings(record=record, module=module) On 2008/08/23 01:46:36, Benjamin wrote:

Why not just do "from warnings import catch_warnings"?

There are other calls to 'warnings' in the module.

http://codereview.appspot.com/3255/diff/1/4#newcode287 Line 287: class WarningMessage(object): On 2008/08/23 01:46:36, Benjamin wrote:

What happened to the idea of making this a namedtuple?

Can't import _collections in a svn checkout since 'warnings' is imported before site.

http://codereview.appspot.com/3255/diff/1/4#newcode320 Line 320: while True: On 2008/08/23 01:46:36, Benjamin wrote:

Can't this be "del self[:]"?

Yep.

http://codereview.appspot.com/3255/diff/1/4#newcode327 Line 327: class catch_warnings(object): On 2008/08/23 01:46:36, Benjamin wrote:

Shouldn't this be called "catchwarnings"?

It's a toss-up thanks to warn_explicit(). I prefer underscores, so I just stuck with it.

http://codereview.appspot.com/3255/diff/1/4#newcode327 Line 327: class catch_warnings(object): On 2008/08/23 01:46:36, Benjamin wrote:

Can't this be a simple generator contextmanager?

No because _collections can't be imported.