Resending, as I forgot to cc: the alias the first time.
- Dermot
-------- Original Message --------
Subject: Re: [caiman-discuss] DC->CUD Project Code review
Date: Wed, 27 Oct 2010 15:36:52 +0100
From: Dermot McCluskey <[email protected]>
To: Alok Aggarwal <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
Hi Alok,
I have only looked at the files you listed in section [2] of your
list.
Comments below.
[2] usr/src/cmd/distro_const/__init__.py
35:
Nit: it seems we should be sorting the imports alphabetically. eg
swap lines 38 and 35.
44:
I know there was some discussion about where is the correct place
to register DOC classes - separately in the modules where they are
defined or all together in the main app.
I thought the correct place was in the modules, not in the app? eg
the Checkpoint class would be registered in the target_spec module?
Did Darren make a statement on this?
usr/src/cmd/distro_const/configuration.py
40:
There are several similar /dev/null streams created in different
modules. Is there a convenient way to combine them all?
67:
why don't you check if type is None, in the same way dest and validation
are tested?
138:
unused variable, msg
110, 121, 128, 139:
How are there messages presented to the user? I think they need some
context. Also, it might be an idea to re-use the *_LABEL variables when
building the message. eg:
err_base = "Error reading <%s> element from manifest" %
Configuration.CONFIGURATION_LABEL
...
...
if not os.path.exists(source):
raise ParsingError("%s: '%s' does not exist: %s" % (err_base,
Configuration.SOURCE_LABEL, source)
etc
130-140:
it appears to me that this method will return an error if:
1. the <validation> sub element is not present (because the
subprocess.check_call()
will fail because path has not been defined?). But according to the DTD,
<validation> is optional.
Am I correct, and if so, is this what you intended?
2. the <validation> sub-element is present, but does not have any path
(again, due to
subprocess.check_call failing, this time because path is None). Is this
also what you intended?
If I am correct, I think the 1st case is an error, but the second might
be valid?
However, in both cases the exception raised by check_call() is not
subprocess.CalledProcessError
so line 138 will not catch it. And, in any event, the msg returned here
("source manifest specified
is invalid...") might be misleading, eg if the path is missing?
148:
'validation' will never be None here, as you initialized it in line 102.
usr/src/cmd/distro_const/distro_spec.py
25:
there is no python doc string for the module here, like in other modules
215:
there's no "return False" here if the tag is not "grub_mods".
I'm surprised this does not cause errors elsewhere?
247:
"lines" might be a better name than "line" here, to show that this attribute
holds multiple <line> elements?
287:
This will return False if <title_suffix> is present but empty, or even
if the *last* <line> is empty, eg:
<grub_entry position="1">
<title_suffix>GRUB Title</title_suffix>
<line>Option #1</line>
<line>Option #2</line>
<line></line>
</grub_entry>
would return False here. Is that what you intended?
307:
"if position is not None:" is better here
309:
"if title_suffix is not None:" is better here
(actually, neither of the above tests are necessary as it would just
re-assign None to the attribute anyway, but I agree it's clearer to
have them there.)
311:
"if len(lines):" is better here (I think?)
usr/src/cmd/distro_const/execution_checkpoint.py
39, 82,
comments are misleading: what "class method" do they refer to?
90:
as the args and kwargs elements are fairly complex in their own right,
my preference would be to have them as separate classes (and probably even
make <arglist>, etc into separate classes also?). As it stands you
are storing up to four levels of sub-elements in one object, eg:
<checkpoint>
<kwargs>
<arglist>
<argitem></argitem>
</arglist>
</kwargs>
</checkpoint>
and you end up with a list inside a dictionary, inside a dictionary,
inside an object!
Plus see comments below, where I think the logic is wrong (probably
due to being overly complex).
118:
it might make more sense to move this line to after 110, so all the
mandatory
attributes are grouped together?
121:
174:
188:
the DTD only allows for 0 or 1 <args> sub-element:
<!ELEMENT checkpoint (args?, kwargs?)>
so you should not need to save and iterate through multiple <args>
196:
in the case where you have more than one <arg> element under a <kwargs>
element, then you will end up repeatedly assigning
kwargs[Checkpoint.ARG_LABEL]
with the dictionary of <arg>s at this line. This works, but it is surely
not optimal?
More reason to split these into separate classes!
205-206:
I believe this is broken in the case where there are multiple <arglist>s
under a <kwargs>
element, as is allowed by the DTD, eg if the input to from_xml() is:
<kwargs>
<arglist name="list 1">
<argitem>item 1</argitem>
<argitem>item 2</argitem>
</arglist>
<arglist name="list 2">
<argitem>item 3</argitem>
<argitem>item 4</argitem>
</arglist>
</kwargs>
then the output from to_xml() would be something like:
<kwargs>
<arglist name="list 2">
<argitem>item 1</argitem>
<argitem>item 2</argitem>
<argitem>item 3</argitem>
<argitem>item 4</argitem>
<argitem>item 1</argitem>
<argitem>item 2</argitem>
<argitem>item 3</argitem>
<argitem>item 4</argitem>
</arglist>
</kwargs>
(I've tested this code on its own and that's what I got).
Yet more reasons to split into separate classes!
213:
This is fine, but for consistency with your other code, you might test if
desc and other non-mandatory attributes are None before the assignment?
217:
"if len(args):" is better, I think?
219:
I'd prefer "if len(kwargs.keys()):" but perhaps Drew can suggest the
most correct
expression to use?
usr/src/cmd/distro_const/manifest/boot_archive_contents_sparc.xml
add CDDL header?
usr/src/cmd/distro_const/manifest/boot_archive_contents_x86.xml
add CDDL header?
usr/src/cmd/distro_const/manifest/dc_ai_sparc.xml
2:
I'm wondering what is the correct location to give for the DTD here?
maybe the eventual install location, "/usr/share/install/dc.dtd" would
be best?
Also, if you don't do so already, it might be an idea to validate the XML
files with "xmllint" from the Makefile, as part of the build?
35 and throughout file.
Only selective lines are indented, eg 35, should be indented; 53 should be
further indented; 56 should be further indented; 113 is over-indented.
Also, some sections have 4-space indents, others have 8-space (eg 295).
I suggest running all the XML files through "xmllint" first to correct
these and other style issues.
296 and elsewhere:
should add a comment to explain the use of DOC search syntax?
297, 301, 308, 465, 472, 476, etc:
closing tag should be merged into opening tag, ie instead an empty
element like:
<image ....></image>
change to:
<image .../>
(xmllint can help with this - except for XML inside comments)
353:
indentation is inconsistent. In fact, I don't think the attributes
should be
split across lines, even if it is allowed. Check what xmllint does with it.
usr/src/cmd/distro_const/manifest/dc_ai_x86.xml
usr/src/cmd/distro_const/manifest/dc_livecd.xml
usr/src/cmd/distro_const/manifest/dc_text_sparc.xml
usr/src/cmd/distro_const/manifest/dc_text_x86.xml
usr/src/cmd/distro_const/profile/ai.xml
usr/src/cmd/distro_const/profile/generic.xml
usr/src/cmd/distro_const/profile/livecd.xml
usr/src/cmd/distro_const/profile/text.xml
Same comments as previous XML files, eg indentation, closing elements, etc.
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
