Hi Ginnie,
Keith and Darren covered a lot of comments I wanted to make. I
tried to not repeat what's already been discussed. However,
if I repeat their comments, please just point me to your responses to them.
* General comments:
- Keith mentioned in his comments about defining a "level" for the progress
records, so the progress handler can fit more smoothly within the logging
paradigm. I thought that's a great suggestion, and I completely
agree with that. I haven't seen your response to that comment yet,
so, I am wondering what you think.
Specific comments:
----------------------
usr/src/lib/install_logging_pymod/install_logging.py
- lines 43-45: Just curious, why do you need to define an
InstallLogException that is not used anywhere else in the code,
just so that it can be subclass by the LogInitException.
Can the LogInitException() just subclass from Exception?
- line 177: I don't think there should be default values for the host
and port arguments. I think host and port should be required arguments.
The ProgressHandler is a client in the server/client model.
The application must start the server, so the ProgressHandler's __init__()
can connect to it in "createSocket()". Since the application must start
the server, it must know the host and port information, so, I think
those 2 values should be required arguments.
- line 182: Looks like tmp_progress_holder variable is only used in the
emit()
function. Is there a reason why it needs to be defined here.
- line 193: is there a reason why the "from install_engine...." line is
here instead of up at the top with the rest of the import statements?
- lines 236-239: TypeError and AttributeError are raised
due to something is wrong in computing the weighted progress
or formating the record. By capturing these 2 errors and printing
them out, we will move on to sending the message, which might not
be correct. Is that intentional?
- line 274: is it really necessary to create a default progress log file?
When the logger initialized, there's no default progress logger. So,
who is ever going to write to the default progress log? When they
eventually add a ProgressHandler, and if they don't specify a filename,
the default progress log will never get written to.
- lines 374-397: I am not sure the value of printing out all
the log file names in the "close()" function. The application would
know all the log file names, since it is the one that added the file
handlers.
As for the location of the default log, if it's value were to be printed,
it would be more useful to print it when logging is initialized.
-------------
usr/src/lib/install_logging_pymod/test/install_logging_unittest.py:
- line 84: Did you forget to remove that line? :-)
- lines 89-97: In the "test_create_defaultlog" test, you just want to
verify that the default log is created. Why not just check it's
existence with os.stat() or something similar.
- lines 114-122: again, check existence of file using os.stat()
- lines 140-149: I think you can do all of these by calling filecmp.cmp()
Thanks,
--Karen
On 07/16/10 12:46, Ginnie Wray wrote:
Hi -
I've posted the python logging files for code review.
http://cr.opensolaris.org/~ginnie/install_logging_pyfiles/
I would like feedback by July 27th, COB.
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
