Hi Keith -
Thanks for taking a look at this. See inline.
ginnie
On 08/13/10 10:19 AM, Keith Mitchell wrote:
Hi Ginnie,
Some final notes & nits on logger.py to finish this out:
63: Since this class now inherits from logging.Filter, it will always
have a levelno - the "or hasattr(record, 'levelno')" can be removed.
you're right. Thanks for catching that.
94-95: Since the __init__ function simply calls into its parent, it
can be removed.
ok.
121: NONE -> None
Yes...
130-135: I believe this function can simply be inherited from
logging.LogRecord now (and so these lines can be removed).
I'll check it out.
189: Despite the prior except clause, this 'bare' except clause is
still a bit 'dangerous'. Recommend changing 187-190 to a single
"except Exception:" clause (which would not catch the
KeyboardInterrupt/SysExit, but would grab everything else).
(See:
http://docs.python.org/release/2.6.5/library/exceptions.html#exception-hierarchy)
255: Seems odd to return anything here.
perhaps. But the C bridge code needs a return value in order to deal
with the failure to report_progress.
280: Non-file handlers will be common enough that it'd be better to
use "hasattr(handler, 'baseFilename')" than a try/except block.
Drew suggested that as well.
- Keith
On 08/ 9/10 04:11 PM, Ginnie Wray wrote:
Hi -
I've incorporated the changes from the last code review.
I would like feedback for these changes by Monday Aug 16th.
Code is located at:
http://cr.opensolaris.org/~ginnie/logger0809/
I've also included output from the nose tests:
bash-4.0$ ./slim_test lib/install_logging_pymod/test --with-coverage
#1 Ensure that FileHandlers can be added to a logger ... ok
#2 Ensure that StreamHandlers can be added to a logger ... ok
#3 Ensure that InstallLogger close works ... ok
#4 Ensure default_log is created and uses the default format. ... ok
#5 Ensure that only one InstallLogger is created for Install Logger
... ok
#6 Ensure that default format is set for the InstallLogger ... ok
#7 Ensure that a log message below the designated level does not log
... ok
#8 Ensure that default log is returned with default_log method ... ok
#9 Ensure that logger name is returned correctly ... ok
#10 Ensure that logger instance is from InstallLogger ... ok
#11 Ensure that debug log messages are logged to the log file ... ok
#12 Ensure that info log messages are logged to the log file ... ok
#13 Ensure that warning log messages are logged to the log file ... ok
#14 Ensure that default log transfers to destination ... ok
#15 Ensure that source file transfers to desired destination ... ok
#16 Test the message returned by ProgressLogRecord get_message ... ok
#17 Test that a second progress handler can be added ... ok
#18 'Tests that progress is reported to the default log ... ok
#19 Test that progress is reported to the progress receiver ... ok
#20 Tests that report_progress fails is value is invalid ... ok
Name Stmts Exec Cover Missing
---------------------------------------------------
osol_install 1 1 100%
osol_install.logger 151 126 83% 73-74, 83, 122, 134-135,
171-179, 187-190, 229, 264, 268, 282-287
---------------------------------------------------
TOTAL 152 127 83%
----------------------------------------------------------------------
Ran 20 tests in 2.300s
OK
Thanks,
ginnie
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
