Edward Pilatowicz wrote:
> https://cr.opensolaris.org/action/browse/pkg/edp/pkg.pli.cr1/webrev/
New modules should have /usr/bin/python for their "interpreter", not
/usr/bin/python2.6.
client.py:
- line 1614: from here forward, you never remove this temporary file if a
failure occurs. I'm also a bit surprised that the actual save() isn't
in a try, even though close() is. Is fdopen() a context manager (ie,
can you use with fobj as os.fdopen() ...."
- __api_plan_load: I'd expect you to use with fobj here.
- line 1623: "it's" -> "its"
- line 2332: shouldn't need "global cmds", since you're not assigning to
it. Though aren't the changes to the other two you're making
irrelevant to the outside context?
- line 2382: should be on previous line
- update(), audit_linked(), sync_linked(), detach_linked(): let's not do
the argument list this way.
generic.py:
- line 122, 128: what does the "j" denote?
- line 720: why move removed_users to the plan description? It's part of
the plan; shouldn't the description be a little less central? Or ...
what's the rationale for putting one object into the plan description
vs keeping it in the imageplan?
client/__init__.py:
- line 73ff: you could use the pop() method on a dictionary to reduce
this a bit.
client/api.py:
- line 380: no apostrophe in "repos"
- line 386: I would expect this to be a decorator, so that you'd have,
for instance:
@__activity_locked_cb
def set_alt_repos():
....
where the content of set_alt_repos() was what's in
__set_img_alt_sources() right now.
- line 397: wouldn't we want to release the activity lock for any
exception? If so, you might be able to structure this as
try:
cb()
except canceled:
cancel_done()
raise
finally:
if locked: unlocked
release activity lock
return rv
- line 1150: should we assert() that we're always under the activity
lock? Of course, that won't be necessary if you make
__activity_locked_cb a decorator ...
- line 1168: where is "be_name" defined? Should this be either
self.__be_name or plan.be_name?
- line 1198: no apostrophe in "its", but an apostrophe in "image's"
- line 1215: no apostrophe in "its"
- line 3916 et al: while you don't need to create a new ImagePlan object
here, you can still keep the shortening to ip. -- fewer changes, and
better readability (IMO, at least where you use it twice; less so in
__calc_frozen()).
imageconfig.py:
- Are there cases where these unicode-to-string conversions would fail?
imageplan.py:
- line 186: the comment expression isn't particularly useful here, and
it'll hide the rest of the assertion. Either ditch it or enhance it.
- line 636: may not need to wrap this line anymore (and maybe other
places where this contraction happened).
- line 2962: no need for the backslash
pkgremote.py:
- line 54: delete
- line 119, 130, 201, 407: "it's" -> "its"
- line 146, et al: Use a verb ("... is the ...")
- line 166: why use real files?
- line 372: "all the other"?
- line 448: "theres" -> "there's"
progress.py:
- You should have some explanation of when _progfd_progress() should be
called, so that future updates to this can be done correctly.
- line 138: why would these happen, and why is it safe to ignore them?
misc2.py:
- I think it's fine that you developed this module to be lint-clean, but
I don't want it shipped as a separate file. Please fold this back into
misc.py once the code reviews are done.
- line 258: what does this mean for unicode action attribute values?
- line 267, 488: "todo" -> "to do"
- line 273: would "if isinstance(desc, dict):" work? You use it (sort
of -- defaultdict is a subclass of dict) in json_decode().
- line 354: "it's" -> "its"
- line 550: shouldn't need frozenset here.
- line 624: no defaultdict? Similarly, on 638: no tuple or set?
pipeutils.py:
- Why use real files?
- line 41: "Servers" -> "server"
- line 48: "Clients" -> "client"
- line 90, 96: delete
- line 175, et al: "recieved" -> "received"
- line 196-199: Would "data = self.__readfh.read(size=size)" work?
- line 200: double quotes
- line 218: the fd you send won't be pointing to the end of the file?
- line 324, et al: "routines"?
- line 343: use "is"
- line 346: No need, given 355
- line 359: should there be a close() here? What __del__() and close()
do in this class are the reverse of what they do in _PipedTransport();
is that on purpose?
- line 614ff: should be moved to the testsuite
prstart.c:
- We get memory information from python by simply reading psinfo. Any
reason we couldn't do the same for prstart?
setup.py:
- line 310: why isn't this pkg.misc2?
- line 554: I think it's about time that we simply loop over ext_modules,
rather than hard-code this for each module. Besides which, this
already isn't correct for Linux.
Thanks,
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
