Jean,
Thanks for looking at this for us.
On 02/27/11 19:06, [email protected] wrote:
Following is the review for chunk c. Most of it is nits, only a few
real questions/concerns. Nice code to read and understand in general.
usr/src/lib/install_target/shadow/Makefile
Copyright should be 2011
Fixed.
usr/src/lib/install_target/shadow/__init__.py
Very nice block comment at the top. The code maintainers will thank you.
Does something like append work? If not, is it worth adding?
Yes. Append can be thought of doing: list.insert(value, -1). In fact,
it's called by the constructor:
for entry in args:
self.append(entry) # will call self.insert
usr/src/lib/insall_target/shadow/shadow_logical.py
line 58: I think it would be nice to put a comment at the top of
insert explaining the exact validation conditions that are addressed.
Great idea. Done.
usr/src/lib/install_target/shadow/shadow_physical.py
lines 139, 255: I think it would be nice to put a comment at the top
of insert explaining the exact validation conditions that are addressed.
Done.
line 196: The comment mentions slice 2, but what does the hasattr
check for?
I know that I had to add the hasattr check because Slice.index is
initialized to None. I can't remember why, though. I'm hesitant to
yank it for not remembering why it was added. I'll watch for it and see
if I can remember why it was needed and drop a comment in.
line 223-227: Is what you're doing here really checking that the slice
doesn't run off the end of it's parent? If so, the comment is
misleading. If not, what are you doing?
Comment changed.
line 234, 245: s/continer/container
Fixed.
line 273: It would be better to use the #define name instead of 15
here and in the following code.
If i remember correctly from the extended partitions project, 15
isn't the only valid value here. Ginnie, William, or Matt should know
for sure.
Fixed to get away from "15". I'll wait and see what other say about
other valid values.
line 302: s/inserted/insert
Fixed.
Thanks, Jean!
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
