Hi Jack,
One comment inline.
On 08/09/11 15:14, Jack Schwartz wrote:
Hi Ginnie.
Thanks for doing this work. I have some concerns with logger.py, but
after talking with you my concerns are "assuaged" :)
For the record, here is my concern:
logger.py:
The idea of a getLogger method which is based on a copy of the
official Python getLogger() method in class Manager is unsettling to
me because it is not maintainable. This idea assumes the official
method won't change and peers into the official method's "black box",
using its algorithm, (quasi-)private variables and methods which start
with an underscore.
Yes, this is just inheritance. However, one usually doesn't play with
private variables of the parent when inheriting from a different library.
From talking with you, I learned that other alternatives to this sort
of thing had been considered in the past and ruled out...
I suppose that if the parent is changed out from underneath our code,
we'll find out fast enough that we need to change our code. This
wouldn't happen unless Python revved their version, and our changes
would just be part of our porting effort.
A more minor concern:
Where does the new getLogger() get called? That's not in this
webrev. Calls to the old getLogger() would need to be modified since
the new getLogger has more args (3) than the original
logging.getLogger(1 arg).
Answer: I think you said that changes to getLogger calls will be a
different webrev...
I talked to Ginnie too about this. I think she will make changes to the
install engine to accept one additional
argument for specifying the default-log-file-name when the engine is
instantiated. The install engine
will setup and call the getLogger() with the new syntax.
This means that the only change that need to be made is for applications
who wants to
provide a default log file to instantiate the engine with that file
name. Applications
will never call getLogger() with the new syntax. Ginnie will leave the
change needed for each
application to instantiate the engine with a default name up to the
application. She won't
do it in this bug fix.
Thanks,
--Karen
test_logger.py:
310-311: remove?
Thanks,
Jack
On 08/ 9/11 12:43 PM, Virginia Wray wrote:
Hi --
Could I get a code review for the following bug:
http://monaco.us.oracle.com/detail.jsf?cr=7033339
Be able to specify logfile name when instantiate InstallLogger
https://cr.opensolaris.org/action/browse/caiman/ginnie/7033339/webrev/
I've tested the changes with the distro constructor. I ran it in its
original
form and it completed successfully. I also modified it to set it's own
default log using my code changes and ran it. I checked to make sure
the engine interface worked correctly with these changes as well.
In both instances, it logged to the location that I expected.
I've included instructions on how to invoke this in the logger.py file.
I've asked Karen and Drew to review. Others are welcome as well.
I'm leaving on vacation, so if you could respond today, I would
appreciate it.
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
