Hi Jack,
Thank you for this review! Responses in-line. New code reviews at:
Full:
http://cr.opensolaris.org/~clayb/12686/webrev1
Differential:
http://cr.opensolaris.org/~clayb/12686/webrev1.diff
Thank you,
Clay
On Fri, 11 Dec 2009, Jack Schwartz wrote:
> Hi Clay.
>
> Here are my comments. Publish_manifest is quite a module.
>
> installadm.c: OK
>
> publish_manifest.py:
>
> 49: Please list in this header the options this function expects to get
> from the commandline. While not "args" per se, the options are still
> expected input by this function and should be documented as such. ...
> and I don't think args[0] is documented in this file.)
Eww, yeah args wasn't documented. I now have:
# since no options are specified simply retrieve the args list
# args should be a list with indicies:
# 0 - service name
# 1 - manifest path to work with
> 57: Just curious: how come OptionParser is still used when you could
> just get args via sys.argv[]? Seems more heavyweight than it needs to
> be, now that there are no options, only args. Seems the maintainability
> benefits you would get (of not having to recode using OptionParser
> later) are muted since the code has to change anyhow if more args are
> added later. Is there some other benefit?
It keeps the Python code the same as other Python code I've seen going
into the gate and code already in the gate. Also, it provides usage
information and options rejection. For example, options parser will
automatically reject any option being provided and print the usage and
quit, etc.
> 69, 85,480, 486: Hard-coded names and strings are hard to maintain. If
> they must be used, I would prefer to see them listed (defined) at the
> top of the file with comments about what they are for. (Some defs
> already exist at lines 43-44, though I'd also like a quick sentence for
> each about what they are for.)
For 480 and 486 I fully agree these should be class variables, not
instance variables
> 491: Manifst -> Manifest
Thank you!
> 514-516: Nit: This doesn't seem to add much value.
Why not? The rest of the initialization function is all setting up XML DOM
variables versus the first half of the init() which is dealing with file
system paths and file I/O?
> 531: Why pass criteria_path into verifyCriteria when it is already part
> of the instance (per line 526)? (Actually I would prefer doing away
> with 523 and 526 instead, but I think pep8 requires all fields to be set
> in __init__, right?)
Thank you, verifyCriteria taking in a file path was vestigial. I've
removed that parameter (and cleaned up resetting self.criteria_path on
line 933).
I don't recall that about PEP8 but I know code reviewers in the past have
been very displeased by not providing all variables in init. Also, the
criteria manifest is an important element; why not keep track of it?
> criteriaRoot is camelCase and criteria_path has a dash in the middle.
> They are not consistent with each other.
Thank you. I've fixed criteriaRoot, AIroot to both be PEP8 recommended
underscore based names instead of camelCase.
> 574: Not sure about "parent". I know what you mean: the DataFiles
> portion of the object, but it is really self, and self is what gets
> passed as parent to __init__ here. Hmmm...
>
> ... I'm wondering if there a cleaner way to do this?
It's not self to my understanding. The _criteria class is its own class.
It has no knowledge of the DataFiles instance which I wish to link it.
Instead of parent I could call it DataFiles_obj?
> Line 578 is pretty wild. I would add a more descriptive comment about what
> it is doing. Something like: "Initialize the parent list class with the
> criteria it stores."
I'd like to reserve "Code Gone Wild"TM for this code review...
I'm not sure what's so wild. This is calling the _init_ for the list class
provided with data by parent.find_criteria() (which returns a generator).
If that's the information you'd like to see I've populated a comment based
on that. Please let me know if you were thinking something else.
> Does it make sense to take the criteria-oriented methods of the DataFiles
> class and make them part of the _criteria child class? If it can be done,
> grouping all criteria-oriented methods together into a single class would
> make things cleaner from both an organizational perspective and probably from
> a code perspective too (i.e. fewer cross-class-calls).
That's an interesting idea! I've made it so.
> General: the code would be much clearer if each method has a description of
> the args it takes, and the values it returns. Please add this information
> for each method in this module.
I've done this. I think for a number of the methods in DataFiles
pre/post-conditions are much more useful than an args/return paradigm for
the particular kinds of effects provided.
> Nit: 570. 581. 590: ''' used instead of """
Is there any difference? Though certainly I should be consistent through
the file; thank you!
> Just curious: how come properties complete with set and get methods are used,
> when there is nothing setting the property after initialization. Seems like
> a regular variable with a get method should do the trick. For example,
> manifest_path is gotten many times but is not set except in
> find_AI_from_criteria() (and the setter isn't used there). Database seems
> another example. Or am I missing something?
There probably could be some merit to cleaning this up. However, all
setters provide assertion errors if something is double set. This was a
style introduced before me (see lines 586-593 in the "old"
publish-manifest.py of the code review)[1]. Otherwise, it provides a
consistent interface which is unavailable if not using properties.
[1]:
I think Ethan set this up in revision 536:ae182deb9b05. Looking at
this as it looks like he was only trying to catch errors from his e-mail:
Date: Wed, 15 Apr 2009 06:15:59 -0700
From: Ethan Quach <[email protected]>
To: Joseph J. VLcek <Joseph.Vlcek at Sun.COM>
Cc: caiman-discuss <caiman-discuss at opensolaris.org>
Subject: Re: [caiman-discuss] code review request for 7986
Though I think a lot of these exception are not necessary, they do allow
change while flagging any potential unintentional behavior for now. As
someone will require justify why they're removing an exception.
> Thanks,
> Jack
>
>
>
>
>
> On 12/08/09 00:57, Clay Baenziger wrote:
>> Hi all,
>> Does anyone have a desire to review a code review heavy on Python
>> properties and refactoring of a sprawling initialization across a program
>> to mostly moved into the appropriate class? It's not too bad really, just a
>> lot of repetitive changes from foo.get_bar() to foo.bar.
>>
>> Webrev:
>> http://cr.opensolaris.org/~clayb/12686/
>>
>> Testing:
>> I've run python by hand by renaming publish-manifest to
>> ./publish_manifest.py and then:
>>>>> import publish_manifest as z
>>>>> import gettext
>>>>> gettext.install("ai", "/usr/lib/locale")
>>>>> b=z.DataFiles(service_dir = "/var/ai/46503", image_path =
>> "/var/ai/install_test_ai_x86", database_path = "/var/ai/46503/AI.db",
>> criteria_path = "/tmp/test.xml")
>> Warning: Using A/I manifest schema
>> </usr/share/auto_install/ai_manifest.rng>
>>
>>>>> b.criteria['arch']
>> 'foobar'
>>>>> b.criteria['foobar']
>> (No KeyError/IndexError, NoneType returned here -- as it should be)
>>>>> b.criteria['mem']
>> ['255', '256']
>>>>> b.criteria['mac']
>> ['c0ffeec0ffee', 'c0ffeec0ffef']
>>
>> As well, I used 'installadm add -m test.xml -n <service>' and the same for
>> a default.xml (setup as a default manifest). I used both range and value
>> criteria in my testing. To exercise things out, to ensure no silent
>> fail-throughs, I intentionally triggered range collisions and exact
>> manifest matches all were correctly triggered.
>>
>> I used the A/I webserver to verify proper filesystem and database output. I
>> used 'installadm remove -m <manifest> -n <service>' to clean-up in between
>> tests.
>>
>>
>> Thank you,
>> Clay
>>
>> PEP8 Compliance Statement for publish_manifest.py:
>> Raw Metrics
>> +----------+-------+------+---------+-----------+
>> |type |number |% |previous |difference |
>> +==========+=======+======+=========+===========+
>> |code |503 |55.40 |NC |NC |
>> +----------+-------+------+---------+-----------+
>> |docstring |149 |16.41 |NC |NC |
>> +----------+-------+------+---------+-----------+
>> |comment |152 |16.74 |NC |NC |
>> +----------+-------+------+---------+-----------+
>> |empty |104 |11.45 |NC |NC |
>> +----------+-------+------+---------+-----------+
>>
>> Messages
>> (I disabled message E0602 as I use 'gettext.install("ai",
>> "/usr/lib/locale")' which provides _() however, pylint doesn't realize
>> that and so goes berserk. I ensured that only can't find function '_'
>> errors were caught by E0602.)
>> +-----------+-----------+
>> |message id |occurences |
>> +===========+===========+
>> |C0103 |26 |
>> +-----------+-----------+
>> |W0212 |9 |
>> +-----------+-----------+
>> |C0301 |5 |
>> +-----------+-----------+
>>
>> Global evaluation
>> -----------------
>> Your code has been rated at 8.99/10
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
>
>
