Hi Clay.
On 12/14/09 13:10, Clay Baenziger wrote:
> 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
Thanks for the differential webrev. It makes things much easier...
>
> 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
Thanks for adding this. Nit: I would prefer to see it as part of the
header (~line 49)
>
>> 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.
OK.
>> 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
OK.
As for 69 and 85, I'll go along for now, as these lines are the only
places where their strings are used in this file. However, they are
used in other files too.
IIRC there is a bug filed to improve sharing definitions between python
programs, am I right? I found 5559 which is more for C<->python sharing
but that's not the same thing.
59: indicies -> indices
111: typo: convience -> convenience
Thanks for adding Args: and Returns in many places. (i.e. at 340-341,
423-425, 444-446, 696, 737, 759) This is incomplete, however. Other
methods/functions could use this as well. for example, 103 (args,
raises); 262 (args); some methods near the end.
711 and 713 are two returns for one method.
725 and 727: ditto
523: typo schmea -> schema
>
>> 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?
OK. Do you want to give it more significance than 518-520 and 533-535
then (by underlining or some such thing?)
>
>> 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).
OK
>
> 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.
OK
>
>> 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.
FWIW, I was referring to line 600 of the old webrev:
self._criteria_cache = self._criteria(self)
Looks like this has changed though...
> 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?
Looks OK now.
>> 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.
Much clearer. All I would change to clarify further is on line 619,
instead of "list" say "the list class".
>> 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.
Thanks.
>
>> Nit: 570. 581. 590: ''' used instead of """
>
> Is there any difference? Though certainly I should be consistent
> through the file; thank you!
Thanks. Yes. Consistency.
>
>> 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.
OK. I went through these kinda fast, but...
Not sure if the set service property is accessed.
Likewise the get of AI_schema.
Rest is fine.
Thanks,
Jack
>
> [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 <Ethan.Quach at Sun.COM>
> 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
>>
>>
>>
