Dermot,
Thanks for the code review. I've addressed a few of these issues, but
the majority of them will be handled by Alok.
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.
I've actually yanked all of the individual register_class() calls and
turned the whole thing into a nice compact for loop which uses the
inspect module to find information for us automatically. Turned 30ish
lines of code into 7 or so.
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?
If we need to relocate the now tiny for-loop somewhere else, we
certainly can. For now, I'm waiting on Darren to comment.
>>> 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?
Alok is planning on addressing some/most of this block.
>>> usr/src/cmd/distro_const/manifest/boot_archive_contents_sparc.xml
add CDDL header?
Done
>>> usr/src/cmd/distro_const/manifest/boot_archive_contents_x86.xml
add CDDL header?
Done.
>>> 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?
We'll be renaming the path to the .dtd files before putback is complete.
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.
I used the --format flag to xmllint to fix the indentation of each XML
file.
296 and elsewhere:
should add a comment to explain the use of DOC search syntax?
Done.
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)
If xmllint changed it for me, that's what I went with. When Alok spins
another webrev, I would greatly appreciate it if you could take a look
and see if you spot any other issues.
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.
xmllint doesn't split across lines, but, imo, that makes the XML even
harder to read. I played with it and Alok and I came up with a 4-space
indent on line continuation. Seems to make the XML mostly ok to read.
>>> 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.
Mostly fixed. I'd love another look when we get the next revision posted.
Thanks!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
