Hi Keith.
Thanks for your review.
On 04/ 7/11 06:46 PM, Keith Mitchell wrote:
mim_errors.py:
94: Based on usage, I think this should be a dual subclass of MimError
and IOError.
OK. Good idea.
115: Based on usage, I think you're looking for "ValueError"? (If that
really won't work for your purposes, then this class should subclass
from ValueError as well as MimError)
OK.
Nit: Why the need for the MimMsgs object? Seems to add one more level
than necessary to define the strings.
OK. Removed the MimMsgs class wrapper around the messages.
Also, I would recommend moving all of the content of this file into
lib/install_manifest_input/__init__.py
While I'm at it, I'd recommend moving mim_utils into __init__ as well.
I don't see a need for a separate "general purpose" bucket - that's
what the __init__ file is for, really.
Hmmm. I suppose so... OK.
mim.py:
160: Use os.environ["AIM_MANIFEST"]
http://docs.python.org/library/os.html#os.environ
(While it makes little difference for getenv(), putenv()/delenv()
don't work as expected, so for consistency, I think it's best to use
os.environ for all access)
OK. I noticed that in some places I had os.environ instead of putenv,
with the comment that putenv didn't work. Now my code is consistent in
that it all uses os.environ.
167: There can be multiple causes for OSErrors. I would recommend
checking explicitly for the ENOENT:
except OSError as err:
if err.errno == errno.ENOENT:
mfest_size = 0
else:
raise
OK. Makes the code more robust.
177: So to clarify, this means that the arg is ignored if we have a
valid self.tree.docinfo.system_url? Can you explain (for my reference)
why this is desired, given that schema_file is a required arg to __init__?
Changed schema_file arg to be optional, and added comment in header
regarding the implications.
191/200/259: Can you explain why there is a need to catch
StandardError here?
191, 259: because the exceptions thrown from etree.DTD() and
etree.parse() are not well documented. (I couldn't find a list of them.)
Changed 200 to catch a MimError.
257: This comment is a bit terse - I'm not sure what you mean here?
Changed to say "Note: errno or strerror are not currently initialized in
IOErrors raised by etree.parse."
280-281: I don't see a strict need to check this here. There are any
number of invalid parameters that could be passed in (True, False,
float, int...); rather than selectively catching just None, it would
be more "pythonic" to just let parse_xml_file - or rather, etree.parse
- choke on them. Python's about letting the caller figure out if what
it passes in is valid or not!
My library API enforces a more strict specification than what the
lower-level libraries allow. The lower-level libraries allow a None
value where I don't. That is why I test for it in some places. In
other places I test to be consistent with the other places where it is
needed.
429-430: Ditto. ManifestInput._path_preprocess should do the checking
(or one of the functions that _path_preprocess calls)
See above.
480: Since list_insert_before is always a list, this "if" clause is
redundant. Assuming the list is empty, the else clause at line 493 is
what will get hit.
Interesting... OK, I've removed the first if and updated the comments.
547-8: See prior comment on checking values.
See above.
573-584: I think this can be simplified to the following (in general,
it should be possible to avoid needing the "index" of something when
using a python list - a worthy goal, anyway!):
left_set = branches[:] # copy of branches, since you need the full
list later
while left_set and IDENT_ONLY_RE.match(left_set[-1]):
left_set.pop()
Cool!
If you happen to need the right_set (you don't seem to keep track of
it here), you could easily instantiate right_set as an empty list, and
change the last line to:
right_set.insert(0, left_set.pop())
Double cool!
Note that the above code negates the need for the else clause on
605-606, and changes the for loop on 618 to:
for branch in right_set:
and that then changes line 674 to:
if branch = branches[-1]
And with that, no need for ugly tracking of the index - now the code
only cares about the branch it happens to be working with!
Wow!
626: Nit: "if insert_before is None"
This code has to be this way. It needs to differentiate an empty list
from no list at all.
651-652: Can you not just do a curr_elem.insert(...) regardless?
Assuming curr_elem is listlike enough, it should readily accept
negative indices for insert.
A negative index isn't what I want, as it would push the last item over.
I tried curr_elem.insert(len(curr_elem), new_elem) and it didn't work.
710: Nit: Since the string here takes a few seconds to mentally parse,
add a comment stating that single- and double-quotes are being stripped.
OK. I added: # Strip leading and trailing single- or double-quotes.
1055-6: Tomato / Tom-ah-to, this could be:
branches[-1] = left
OK.
(A similar change could be made on lines 1151-2)
Yup.
1081: Nit: Need a space after the comma
OK.
1110, 1142: I'm tempted to say these could possibly be optimized to
not need to track idx, but it would be a big enough change that it's
not worth the risk or time. However, 1142-3 can be changed to:
for idx, branch in enumerate(branches):
(Thus removing the need for line 1143)
OK, thanks.
1208: Nit: No need for parens around return values.
OK.
Thanks again for your review.
Webrev respun:
delta:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.3.4.diff
vs slim_source:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.4
Jack
On 04/ 7/11 05:14 PM, Jack Schwartz wrote:
Hi Keith.
Thanks for offering to take a look.
The files are the delta changes on the AI-client side:
usr/src/lib/install_manifest_input/mim.py
usr/src/lib/install_manifest_input/mim_errors.py
of review:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.2.3.diff/
Background:
This is the manifest input module. The delta changes regard pathing
fixes. Pathing code converts the paths specified on input, into
xpaths. Here's a breakdown of the changes:
General:
1) Get rid of self.no_starting_data, as a check for not self.tree
does the same thing.
ManifestInput:__init__():
1) move the schema processing to below where the tree is
instantiated, so it can retrieve the schema from the tree (from the
XML file).
2) Move XML parsing code into its own function, so as to not
replicate exception handling, etc.
ManifestInput:parse_xml_file():
XML parsing code function.
ManifestInput._path_preprocess():
This function is the highest-level path processing function. add,
get, set all call it to get an xpath. Before, it returned an xpath
to the relevant element, plus the name of any attribute given at the
end of the path.
One change is to allow a path with a value assigned to a final
element. Xpath doesn't process paths with final values (e.g.
/a/b=5), so the final value must be stripped from the returned xpath
and returned as a separate arg. In this example final_val will
return 5 and xpath will be /a/b. _path_preprocess() calls
_strip_final_value() to take care of this.
ManifestInput._xpath_search():
This does the xpath search. It has been changed to accept the
final_val argument, which it now uses to find the desired final
element based on the final_val passed in, if multiple final elements
are found by the xpath search.
Note that we only want to strip the final_val if it is not in [].
xpath processes final values if they are in []. This is why on 593
and 1139 we check for the [].
Other misc things:
564: this is added so, for example, if a tree that starts with
/auto_install
is present, one cannot add another tree with a different root, like
/auto_install_blah/....
1098: this is added so paths with only / in them will fail.
We can chat in the morning about this if you have more questions.
Thanks again for offering to review.
Jack
-------- Original Message --------
Subject: Please review: bugfixes and code review comment integration
for Derived Manifests
Date: Tue, 05 Apr 2011 17:19:10 -0700
From: Jack Schwartz <[email protected]>
To: caiman-discuss <[email protected]>, Matt Keenan
<[email protected]>, Drew Fisher <[email protected]>, Karen
Tung <[email protected]>, Ginnie Wray <[email protected]>,
John Fischer <[email protected]>
CC: Dave Miner <[email protected]>, Li Ming <[email protected]>
Hi everyone.
Here is hopefully the last round of webrevs with significant changes
for Derived Manifest. (Li is still testing but is almost done. He
needs "final" bits to complete his testing.)
AI server changes are minimal, mostly fixes to bugs found by Li of QE.
vs previous webrev:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.srv.3.4.diff/
vs slim_source:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.srv.3/
AI client changes are a little more substantial, including bug fixes
found by Li, re-whacking of aimanifest for better logging, and
bugfixes to pathing.
vs previous webrev:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.2.3.diff/
vs AI-CUD gate*:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.3/
* Note: I pushed to AI-CUD gate at the time of first code review, so
the AI-CUD team could use my bits for testing.
I would be grateful for the following people to help with the review
as soon as possible. If you are named and can't review please let me
know ASAP.
Client side:
Matt: changes to dmm.py and new dmm_log_test unit test.
Drew: changes to mim.py and mim_errors.py
Karen: changes to aimanifest.py and packaging
Ginnie: changes to logger.py
Note: Dave M. blessed changes to rbac files before going on vacation.
Server side:
John: all changes
Thanks everyone for your time,
Jack
P.S. Notes: AI client webrev shows changes for auto_install.py in the
delta, but they are not my changes; ai_get_manifest.py is reverted
to what was there before I pushed originally.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
