Hi Keith,
Please see my responses inline. I have removed all sections
where there's no further discussion needed.
On 09/28/10 10:32, Keith Mitchell wrote:
On 09/27/10 05:02 PM, Karen Tung wrote:
Hi Keith,
Thank you for the code review. Please see my responses to your comments
inline.
On 09/23/10 11:56 AM, Keith Mitchell wrote:
[...]
install_common.py:
I'm not a fan of ambiguous names like "common" and "utils" for files.
Too easy for them to become dumping grounds for random code. Is there
any reason not to put INSTALL_LOGGER_NAME and get_argspec in
engine/__init__? I think most consumers of that variable will have the
engine anyway (and by virtue of importing
"solaris_install.engine.install_common" they'd be importing
"solaris_install" and "solaris_install.engine", so there's no memory /
performance gain by separating this).
Unfortunately, we can not do it in this case. It will create a circular
dependency between __init__.py
and CheckpointData.py. __init__.py needs to import the CheckpointData
object from CheckpointData.py,
but CheckpointData.py needs to import INSTALL_LOGGER_NAME and
get_argspec() from __init__.py.
So, looks like we really can not get rid of this file. We can give it a
better name. Do you have any suggestions?
Hrm, the circular dependency is a red flag that maybe this does need
to change somehow, but I'm not sure as to what might be best. Some
possibilities:
* INSTALL_LOGGER_NAME could feasibly go into
solaris_install/__init__.py or solaris_install/logger/__init__.py; and
* get_argspec could get moved into checkpoint_data.py, or into the
existing install_utils module (if we're going to have a dumping
ground, might as well restrict it to one location); or
* Could also rename the file, but I can't think of anything
appropriate off the top of my head.
What I have decided to do to get rid of the install_common.py file.
The INSTALL_LOGGER_NAME is move to the file where InstallLogger class
is defined. So, all components will get the name via:
from solaris_install.logger import INSTALL_LOGGER_NAME
The get_argspec() function is moved to
usr/src/lib/install_utils/install_utils.py file.
That file currently has a lot of common functions that's used by various
install
components. Since get_argspec() sort of a common function, it belongs
there.
666: So, why use a Decimal but then cast it to an int?
cp_data.prog_est_ratio is a floating point number. So, the
normalized_prog will be a floating point value
if it is not cast to an int. I think it will be easier to return the
normalized progress
as an int instead of a floating point number.
I suppose what I'm asking is, why not return a Decimal?
When we define the interfaces, we decided the normalized_progress()
function will return a string. I supposed we can change the interface
for it to return a Decimal too. The logging module which
uses this function will just convert
the Decimal to a string anyway before reporting the value to
the registered handlers. So, I thought it will just be easier to do
the conversion to string so I don't need to change the logging module.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
