Ginnie,
I only looked at logger.py
logger.py
========
General Nit: I would like to see you standardize how you are doing
your function/method docstrings. Sometimes you use single ticks,
sometimes you use double quotes, sometimes you use hash marks,
sometimes you start your comment on the same line as the ticks/quotes,
sometimes you don't, etc.
42: User-Created Exceptions should end in "Error", so change the
class name from LogInitException to LogInitError
53-54: extra space on indent
85-86: When you call the parent's constructor, don't assign variables
unless you have to. Pass the variables from the subclass to the
parent or people that use your class can't set variables
logging.FileHandler.__init__(self, filename=filename, mode=mode,
encoding=encoding, delay=delay)
if you NEED the values hardcoded, explain why in a comment.
98: don't use 'is' here. Very dangerous. Since you're testing for
an integer, use == instead
103-104: combine into one line
106: I don't think the backslash is required here.
125-128: combine into one line
156: remove this line
168, 196, 275: add a space after the #
204-205: these lines are not needed because they're covered in the
parent's __init__ call.
210-211: I see the differences in the two lines, but why are you
doing this? Can you comment it?
267-268: this isn't needed as you can use os.path.join in line 269
to take care of putting the "/" in if needed
277, 285-287: The except clause should never trigger, should it? if
the .values() call returns an empty list, nothing happens in the for
loops.
279-284: why is this try/excepted? You're not doing anything that
would raise an exception?
I think that's all for now!
-Drew
----
On 08/09/10 17:11, 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
