On 05/22/12 01:45, Edward Pilatowicz wrote:
hey all,
so i've finally got the parallel linked images code ready.
you can find the webrev here:
https://cr.opensolaris.org/action/browse/pkg/edp/pkg.pli.cr1/webrev/
the webrev contains a up-to-date design doc which is probably a good
place to start.
----------------------------------------
src/client.py:
----------------------------------------
line 317: I don't think you need this assignment at all if there's no
usage text for the command. However, if it is needed (we should fix
that), you can at least drop the _() here.
line 2328: docstring please
line 5237: brief docstring please
line 6076: please describe data structure in comment
line 6431-6432: This should not be doune outside of main_func or
handle_errors. If an exception happens while msg() is outputting
str(pkg_timer)),our internal exception handler won't catch it. If you
must do this here, then wrap it with a call to handle_errors.
----------------------------------------
src/man/pkg.1:
----------------------------------------
line 337: s/the/the maximum/ ? Also, perhaps s/child images/zones/ ?
Realistically, we aren't going to be using this for anything else and
most users will likely stumble over "child images".
line 1987: Similar to 337, except s/number/maximum number/ ?
----------------------------------------
src/modules/actions/generic.py:
----------------------------------------
line 132: insert another newline here
----------------------------------------
src/modules/actions/license.py:
----------------------------------------
line 78: there's no comment explaining this, but my guess is that
this is a side-effect of plan serialisation. What was the reasoning
behind this particular approach?
----------------------------------------
src/modules/client/__init__.py
----------------------------------------
line 61: used by whom or what?
line 64: s/used/used by/ ?
----------------------------------------
src/modules/client/api.py
----------------------------------------
line 380: s/repo's/repos/
line 380: can you explain why this change is needed? the comment
should. I purposefully deferred the loading of the catalog until needed
for performance / memory usage reasons.
lines 471-475: This can be simplified to "misc.makedirs(plandir)";
no try/except or isdir check needed
line 1126: s/json a copy/json copy/
lines 1143, 1192: s/PlanDesciption/PlanDescription/
line 1145: suggest this instead: "Prevent loading a plan if one has
been already."
line 1178: s/unknown fmri/fmri part of plan, but currently unknown:/
line 1198: s/images/image's/
lines 1198, 1215, 1230: s/it's/its/
line 1199: s/. Then/; then/
line 1289: I don't like exposing pubcheck as part of the general API;
is there a way to have this automatically determined so we don't
have to expose an implementation detail to callers?
line 1290: I can't find where "show_licenses" is actually used, and
I'm also confused as to what purpose it serves. If it's similar
in purpose to 'accept', it needs a docstring, and now we have an
inconsistency in naming for 'accept'.
lines 4866-5085: A comment needs to be added to pkg.client.plandesc
about changes requiring synchronisation with API version bumps. It's
also unfortunate that this move means even less of the interface is
readily accessible through 'pydoc pkg.client.api'. With that in mind,
add a sentence similar to line 30 that refers to pkg.client.plandesc.
----------------------------------------
src/modules/client/imageplan.py
----------------------------------------
line 116: This implies that we always go through plan serialisation;
that's unexpected. Please ensure that we don't go through plan
serialisation unless linked images are involved.
line 433: need a newline before this
----------------------------------------
src/modules/client/pkgremote.py:
----------------------------------------
lines 119, 130, 201, 407: s/it's/its/
lines 177-182, 231-236, 444-446, 469-471, 491-494, 503-508, 559-562:
seems like these need to be try/finally to ensure lock gets
released even if execution is interrupted
Someone else should review the threading code here too.
----------------------------------------
src/modules/client/plandesc.py
----------------------------------------
line 65: I would suggest prefixing the class name with '_' as we
don't intend for general consumers to use this and make that clear in
the docstring.
line 67: s/image/image-/
line 101: insert additional newline here
lines 190-229: class declarations should be before class methods;
(move this before __init__ please)
lines 292, 308: I don't think we want general API consumers calling
these methods. I suggest making them protected by prefixing with '_'.
lines 421, 477, 501: why do we have a method that just simply returns
a property value?
line 603: s/we will update indexes/indexes will be updated/
line 640: s/image/image-/ ?
----------------------------------------
src/modules/misc2.py
----------------------------------------
Not thrilled about having a separate module only because of
pylinting :-(
line 33: s/are/is/
line 108: s/ out// ?
lines 143, 144, 145, 148, 584: s/pointer/reference/
line 143: s/. this/. This/
line 168: s/top level/top-level/
line 168: s/encode/encode. / ?
lines 250, 514: I don't see where 'type' gets assigned, and I don't
think we should have a variable named the same as a builtin function either.
lines 295, 313, 519, 537: s/items()/iteritems()/
line 354: s/it's/its/
line 464: actually, it does support unicode; see the various
catalog tests where I verify unicode support as an example
line 579: s/an/a/ ?
----------------------------------------
src/modules/pipeutils.py:
----------------------------------------
nit: Two newlines after each class definition.
line 399: I think this needs a 'if self.__pipe_file' condition
line 405: missing self.__pipe_file = None?
line 548: s/. """/."""/
----------------------------------------
src/modules/prstart.c
----------------------------------------
Weird to have a whole module just for this function
carved out of the namespace.
lines 47, 49, 53, 55: are the braces necessary?
The timings stuff also seems off somehow. For example, running 'pkg
update -nv' on my system takes a total of 7.00s user, but timings
reports a total of 0.217s user. It looks like it's only timing client
startup.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
