Hi Jack,
Comments in-line and only to things not followed. New webrevs at:
http://cr.opensolaris.org/~clayb/12686/webrev2
http://cr.opensolaris.org/~clayb/12686/webrev2.diff
Thank you,
Clay
On Mon, 14 Dec 2009, Jack Schwartz wrote:
> 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)
I really appreciate this being near where the code is so that it won't
become stale. One of the bugs here in fixed was a mismatch between the
usage and use of args already.
>>
>>> 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
These are differences between the "Returns:" block and the function's
descriptive sentences.
> 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)
I should have called _criteria_cache self too...
> 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
>>>
>>>
>>>
>
>
>
