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.)
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?
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.)
491: Manifst -> Manifest
514-516: Nit: This doesn't seem to add much value.
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?)
criteriaRoot is camelCase and criteria_path has a dash in the middle.
They are not consistent with each other.
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?
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."
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).
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.
Nit: 570. 581. 590: ''' used instead of """
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?
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
