On 07/12/12 17:53, Danek Duvall wrote:
Brock Pytlik wrote:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/7139873-v1
bootenv.py:
- I think we need some brief documentation explaining what the three
image variables are, and how they interact. Getting it right is
critical, and having to figure out what needs to happen by reading the
code is difficult and error-prone at best.
That's a good idea. I'll add that.
- line 75: space before equals. I'd also call this img_set_cb() or
something that indicates that it's a callback function.
Another good idea.
- line 148: I know that the one place that uses this mechanism does it
correctly, but in the general case, is there a real requirement for
img_setter to be non-None? Why not just call it only if it's non-None?
I can take the assert out if that's preferred. Since there's only a
single use case for it, I thought it made more sense to require it to be
set, but I don't really feel strongly about it.
- line 152: would it be correct and not confusing to have current_img
simply return self.clone_img when that was not None, and self.orig_img
otherwise?
It would be correct as far as I can see. I'm not sure about the
confusing part one way or the other, but I think I do like your approach
better since it means that current_img can't be set to something other
than orig_img or clone_img. I'll switch to your idea.
- line 224: do we need this assert on img_setter here, too (even if we
keep the other)?
Probably not. I think I may have put this in to help me during debugging
and had forgotten that was the use of the assert.
- line 431: if this is the case, then why bother passing img as a
parameter?
Good question. I'll remove the parameter.
- line 432: use "is"
Ok.
image.py:
- line 259: space after colon
- line 261: probably just clone()
imageplan.py:
- line 123: space after colon
These all sound good to me.
Thanks,
Danek
Thanks for the review,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
