Brock Pytlik wrote:
> Here's the link:
> http://cr.opensolaris.org/~bpytlik/ips-api-v1/
>
> I haven't done as much cleanup as I usually try to do, but since I'm
> ooto tomorrow, I thought it would be better to get this out now, rather
> than next Monday.
>
> Other things that I know need to be improved, but that I think can wait
> for another round of revisions:
> API Exception organization
> More complete functionality
> Automated testing of cancellation (I hope to work with the GUI team to
> test this by hand in the interim)
> Versioning of the Progress tracker (perhaps)
> More Documentation
> Figure out the future approach to testing we'll take
client.py
line 49: unused import, re
line 62: unused import, imageplan
line 65: unused import, bootenv
line 68: Why aren't the search errors part of the api exceptions now
as well?
lines 397, 520: This should probably be part of the api now (not sure
where to put it) instead of tweaking this class directly. That would
also make the import on line 63 unnecessary.
line 415: It would be nice if every place you passed in "0" for a
version_id, it was a constant instead set at the top of client.py such
as CLIENT_API_VERSION or some such.
line 416: "We are the knights who say Ni!"
line 424, 435: I would change the message here, even though it should
never happen, let's not print something that says that :-) A better
message might be: "Unexpected failure during image-update while
determining what to update."
line 439: Perhaps this call to display_pce should be a print that
takes advantage of a __str__ method of PlanCreationException?
line 842: Message could use some tweaks: "Encountered unknown package
information state: %d" % pi.state
line 854: Looks like you could use misc.bytes_to_str here. Which
would also mean you could remove line 862.
line 1239: instead of reassigning temp here, why not just wrap the
_() around line 1236-1238?
line 1476: This doesn't appear to be valid anymore?
line 1534: --no-refresh is still a valid option, why remove this?
lines 1668, 1672, 1674, 1676, 1684: You should pass img.imgdir here
instead of mydir as the image object could have a different value for
img.imgdir (see lines 190-191 of modules/client/image.py).
line 1713: it seems like we should call api.cancel() here in the
event of a KeyboardInterrupt or a KeyboardInterrupt should automatically
trigger a cancel in the api itself somehow.
pkg.1.txt:
lines 99, 120: s/its catalog/their catalogs/ or s/its catalog/catalogs/
api.py:
line 1: missing #!/usr/bin/python2.4
line 2ff: missing cddl license header block
line 2ff: I'd turn most of your comments after the def blah(...) into
docstrings for the related functions. docstrings for all your classes
would be nice too.
line 48: Nitpick: s/cancelling/canceling/ to be consistent with
"canceled" line 47, et al. American vs. British English :)
line 72: Shouldn't this be an "else:" case for line 74 to avoid a
double load of catalogs in the event they're refreshed?
line 90: drop spaces around '=' for PEP8
line 192: drop
line 513: drop spaces around '=' for PEP8
line 514: why assign this to ret and then return ret? Just return {}
directly.
lines 515-518: indent 4 spaces not 8
line 591: UNINSTALLED doesn't seem right here. I'd use the states
presented in doc/pkg-states.txt, which in this case would appear to be
DELETED if it was previously installed or IN_CATALOG if it's never been
installed.
api_errors.py:
line 1: missing #!/usr/bin/python2.4
line 1ff: missing cddl header block and docstrings for all your
classes / functions would be appreciated.
line 1: anything that inherits from exception should have it's own
__init__ or have its own __init__ and call exception.__init__
image.py:
line 1954: drop
line 1956: drop spaces around ppat
line 1985: s/start/end/
lines 1993-1994: Mainly it's just the -- that seems awkward. However:
s/--which we're going to alter,/(which will be altered)/
lines 2037, 2038: drop the commented lines
lines 2062-2067: indentation is "wonky" here
lines 2080, 2081: drop
misc.py:
lines 314, 319: why are these commented?
line 324: drop
=====================
Overall, I'm really happy with how this turned out. Bravo!
I just want to be clear that I was never against having a "refined API".
My belief is that we have two apis in our gate: a "blessed API" and a
set of "private APIs".
In my opinion, all APIs should work for all code in our gate. However,
code outside of our gate should only use "blessed APIs".
This means to me that yes, any code that calls uses any of our private
APIs that is in our gate should be updated to reflect changes being made
in those private APIs. However, usage of private APIs by client code in
our gate should be strongly discouraged.
As such, I'm looking forward to seeing this API you've taken an initial
stab at expanded in the future to be more inclusive.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
