On Tue, Mar 13, 2012 at 1:09 AM, Greg Stein <gstein_at_gmail.com> wrote:
> On Tue, Mar 13, 2012 at 00:55, <hwright_at_apache.org> wrote:
>>...
>> +++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Tue Mar 13 04:55:26 2012
>> @@ -36,6 +36,7 @@ import optparse
>> import xml
>> import urllib
>> import logging
>> +import cStringIO as StringIO
>
> No need to rename this module, as is typical with back-compat importing.
This isn't backward compat, it's an alternative implementation that is
a drop-in replacement for the StringIO module, hence the desire to
alias it. Though I've also noticed that in some places we *do* import
just the StringIO class from those modules, and I'm tempted to do that
here for consistency.
Actually, it's moot because of the other changes made to fix the
following issues.
>
>>
>> try:
>> # Python >=3.0
>> @@ -993,6 +994,15 @@ def use_editor(func):
>> os.environ['SVNTEST_EDITOR_FUNC'] = func
>> os.environ['SVN_TEST_PYTHON'] = sys.executable
>>
>> +def _log_exception():
>> + import traceback
>> +
>> + o = StringIO.StringIO()
>> + ei = sys.exc_info()
>> + traceback.print_exception(ei[0], ei[1], ei[2], None, o)
>> + logger.warn(o.getvalue())
>> + o.close()
>
> You could simplify: logger.warn(traceback.format_exc())
>
> I would also recommend importing traceback at the module level, rather
> than within this function. I've seen cases where the "import
> traceback" will *overwrite* the exception registered on the current
> thread. Something as simple as Python looking for the traceback module
> and doing an open("/some/weird/path/traceback.py") and getting an
> IOError. The above code likely *happens* to work because traceback is
> already registered in sys.modules, so a true import doesn't have to
> happen.
>
> I would also recommend that _log_exception() takes parameters so that
> you can collapse the logging into a single invocation, all going to
> logger.warn(). The current code allows the caller and this function to
> (accidentally) use different logging levels.
>
> def _log_exception(fmt, *args):
> logger.warn(fmt, *args)
> ...
This stuff fixed in r1300118.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Received on 2012-03-13 14:34:59 CET