[Delivery to c-d failed - resending]
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.
checkpoint_data.py:
113: I believe this should be __eq__, as __cmp__ is used to define
both equality and sorting mechanisms. An AttributeError may be raised
if a comparison of CheckpointRegistrationData == "some string", so
this should be caught and 1 returned if that happens.
While I was writing the code, I implemented this function as __eq__, but
I don't seem to be
able to get the comparison to work. I found that it works
if I do it as __cmp__. Now that you mentioned it, I looked at it
again. After some search, I found that since I am
doing the comparison using "!=", the __eq__() function doesn't get
called. The __ne__() gets called instead.
If I don't have the __ne__() defined, the __cmp__() function is called.
That's why __cmp__() works for me.
Now, I restored the __eq__() function I had, and implemented a __ne__()
to return "not self.__eq__()".
Perfect!
[...]
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?
[...]
833: I think it should be possible to just do "Decimal(prog_est)"
without the intermediate cast to string.
837: I think this could be "Decimal(0)" though I'm not sure how much
of a difference it makes.
If the value is an integer, you don't need to cast to string first. If
the argument for the Decimal() constructor
is a floating point number, then, it needs to be converted to string.
a=1
b=1.1
decimal.Decimal(a)
Decimal('1')
decimal.Decimal(b)
Traceback (most recent call last):
File "<stdin>", line 1, in<module>
File "/usr/lib/python2.6/decimal.py", line 649, in __new__
"First convert the float to a string")
TypeError: Cannot convert float to Decimal. First convert the float to
a string
decimal.Decimal(str(b))
Decimal('1.1')
Aha, I see now. Thanks for the explanation.
For line 833, I think I will keep the cast to string, just in case
get_progress_estimate() returns a non-integer
for some reason. For line 837, I will change to Decimal(0).
[...]
Thank you again for the code review.
--Karen
You're welcome!
- Keith
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
