Hi Drew.
On 03/28/11 08:49 AM, Drew Fisher wrote:
I need way more coffee....
-Drew
Thanks for slogging through this... I do appreciate it. Here is my reply:
-----------------------------------
mim_utils.py
-----------------------------------
76: why not use None?
Jack: OK.
77-91: could you use an else: clause at the end of the for-loop?
for idx in range(...):
...
...
if not (double_active or single_active):
if curr == ch:
break
...
...
else:
idx = -1
return idx, brkt_active_level
Jack: OK.
112, 113: no need to predeclare
Jack: yes I do. I pass brkt_active_lvl and idx into a function on line 117,
and reference start_idx is referenced on line 120. These variables all need
to be set before they can be used.
127: use += (idx += 1)
Jack: OK.
132-149: I'm really not a fan of 2 line functions. It adds
unnecessary lookups to the code (both by the interpreter itself and
anybody supporting the code). I only saw two places in mim.py where
branch_split() is used (and one was a smokin' hot list comprehension),
so you may not be able to easily remove it.
Jack: Right now, there is nothing that calls noquote_nobkt_split() except
branch_split(). Originally I separated these out because
noquote_nobkt_split()
was more general. If a bug I have to fix doesn't preclude combining these
two functions, I will do so after I fix the bug. So for now, I've noted
this
and will get back to it later.
-----------------------------------
mim.py
-----------------------------------
Good lord. I'll leave it at that. XML, regex, and list slicing. Ow.
65-98: I was totally unprepared for regex with an uncaffinated mind.
I'm going to have to take your word for it on these. :)
Jack: Sorry... OK.
176-177: replace with os.path.getsize(self.manifest_name) [us python
people fear stat(2) calls. :)]
Jack: OK.
208: change to: if self._warnings is None:
Jack: Done.
215-223: You may want to comment the reason why you reset
self._warnings to None
Jack: OK. I added the following to the method header:
Since messages accumulate in the cache, they are returned only
once.
450: is this a test for an empty list? If so, simply check via
boolean:
if not list_insert_before:
insert_at_idx = -1
Jack: OK. I also added comments saying that list_insert_before cannot
be None,
to avoid confusion. (In other parts, None has a different meaning than
empty-list.)
453-467: nice. for/else :)
Jack: Thanks!
592, 795: use boolean. if not curr_elem:
('', 0, [], (), {} all evaluate to boolean False)
Jack: Done.
621-625: can you use enumerate() instead of range()?
Jack: I need the numeric for calling insert() on lines 626-627.
663: simply make the dictionary a 4-space indent.
Jack: Hmmm... I see it is mis-indented, but shouldn't it be one char to the
right of the ( on the previous line as it is part of the stuff inside
the ()?
928: amassed
Jack: Thanks.
980: enumerate() ?
Jack: OK. Replaced:
for i in range(len(overlay_element)):
child = overlay_element[i]
with
for child in overlay_element:
1023: use boolean for the if conditional.
branches = [branch for branch in branch_split(path) if branch]
(haha, say that out loud. it's almost a tounge-twister)
Jack: Done.
1038: boolean
Jack: OK.
1074-1117: my brain broke. :)
Jack: Sorry... Hopefully at least the comments were helpful...
1085: use rpartition
Jack: OK. Also changed lines 1067 and 1123
>>> s = "a[b/c@attr=val]"
>>> left_eq, _none, right_eq = s.rpartition("=")
>>> left_eq
'a[b/c@attr'
>>> _none
'='
>>> right_eq
'val]'
This allows for splitting a string exactly once. the "r" in
rpartition means, "read from the right"
Jack: Thanks.
1092: use partition
Jack: OK.
>>> left_of_open_bkt, _none, right_of_open_bkt = left_eq.partition("[")
>>> print left_eq.partition("[")
('a', '[', 'b/c@attr')
1095 - 1099: use partition
>>> left_at, _at, _right_at = right_of_open_bkt.partition("@")
>>> print right_of_open_bkt.partition("@")
('b/c', '@', 'attr')
partition is really cool for doing splits like this. in the last
partition (with the "@" character), if that "@" character isn't in the
string, it won't error:
>>> "this string has no at symbol".partition("@")
('this string has no at symbol', '', '')
Jack: Thanks for the explanation.
so, the try/except on 1096 turns into a conditional
if not _at:
no_at = True
Jack: Even better, I can get rid of no_at altogether, and change this:
try:
left_at, right_at = right_of_open_bkt.split("@", 1)
except ValueError:
no_at = True
if no_at == False and left_at != "" and right_at != "":
to:
left_at, _at, right_at = right_of_open_bkt.partition("@")
if _at and left_at and right_at:
(I need all three, to handle strings which start with @.)
1101-1104, move this to line 1084 (before all the partitions)
Jack: Done!
Thanks,
Jack
On 3/26/11 4:10 PM, Jack Schwartz wrote:
Hi Drew.
Thanks for the reviews you submitted for Derived Manifests. I had a
lot of changes to make, and I'm retesting things before I respond and
repost.
I thought you volunteered for section C at the staff meeting before
last. If I am mistaken or you cannot do this, please let me know
ASAP. Otherwise, I'd like the feedback ASAP. Either way please let
me know.
Thanks,
Jack
P.S.: I haven't updated the webrev yet, but I've already done things
like remove extraneous () and order imports as you suggested in your
prior review.
On 03/11/11 07:36 PM, Jack Schwartz wrote:
Hi everyone.
Here is the first of two code review wads for the Derived Manifests
project.
This wad includes the AI client side support for running a script to
derive manifests. A subsequent (much smaller) one coming within a
few days will be for improved AI server support of default manifest
management.
The focus of this code review is on the following:
- the Derived Manifest Module checkpoint
- the aimanifest command
- the Manifest Input Module (which supports aimanifest)
http://cr.opensolaris.org/~schwartz/110311.1/webrev/index.html
Please take a group of files and review by Weds 3/23 COB. Please
let me know ASAP what you are able to review.
I'll be setting up an info session for Tuesday morning from 9-10 PST
to help *jumpstart* the process. (Sorry, couldn't resist) Logistic
info to follow.
I suggest the following groups:
(A) The Derived Manifest Module
usr/src/cmd/auto-install/checkpoints/dmm/Makefile
usr/src/cmd/auto-install/checkpoints/dmm/__init__.py
usr/src/cmd/auto-install/checkpoints/dmm/dmm.py
usr/src/cmd/auto-install/checkpoints/dmm/dmm_errors.py
usr/src/cmd/auto-install/checkpoints/dmm/test/Makefile
usr/src/cmd/auto-install/checkpoints/dmm/test/dmm_build_test.py
usr/src/cmd/auto-install/checkpoints/dmm/test/dmm_env_test.py
(B) The Manifest Input Module, focus on overlay
usr/src/lib/install_manifest_input/Makefile
usr/src/lib/install_manifest_input/__init__.py
usr/src/lib/install_manifest_input/process_dtd.py
usr/src/lib/install_manifest_input/test/test_manifest_input_overlay.py
usr/src/lib/install_manifest_input/test/test_process_dtd.py
(C) The Manifest Input Module, the rest
usr/src/lib/install_manifest_input/mim.py
usr/src/lib/install_manifest_input/mim_utils.py
usr/src/lib/install_manifest_input/mim_errors.py
usr/src/lib/install_manifest_input/test/test_manifest_input_pathing.py
usr/src/lib/install_manifest_input/test/test_manifest_input_set_get_add.py
usr/src/lib/install_manifest_input/test/test_manifest_input_validate_commit.py
(D) aimanifest command, AI environment prep, packaging and other files:
usr/src/cmd/aimanifest/Makefile
usr/src/cmd/aimanifest/aimanifest.py
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/cmd/Makefile
usr/src/cmd/Makefile.cmd
usr/src/cmd/Makefile.targ
usr/src/cmd/auto-install/auto_install.py
usr/src/cmd/auto-install/checkpoints/Makefile
usr/src/cmd/auto-install/config/get_manifest
usr/src/cmd/auto-install/svc/auto-installer
usr/src/cmd/auto-install/svc/manifest-locator
usr/src/lib/Makefile
usr/src/lib/Makefile.targ
usr/src/lib/install_target/Makefile
usr/src/lib/install_target/shadow/Makefile
usr/src/pkg/manifests/system-install-auto-install-auto-install-common.mf
usr/src/pkg/manifests/system-install-auto-install.mf
usr/src/tools/tests/tests.nose
usr/src/cmd/rbac/Makefile
usr/src/cmd/rbac/prof_attr.system%2Finstall%2Fauto-install
usr/src/cmd/rbac/user_attr.system%2Finstall%2Fauto-install
usr/src/cmd/auto-install/ai_get_manifest.py
Thanks,
Jack
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
