Hi Keith,
Thank you for the review! Comments in-line. New webrevs at:
http://cr.opensolaris.org/~clayb/python2.6/webrev2_diff/
http://cr.opensolaris.org/~clayb/python2.6/webrev2/
Thank you,
Clay
On Thu, 5 Nov 2009, Keith Mitchell wrote:
> Hi Clay,
>
> My usual disclaimer applies - please don't hesitate to defer/ignore
> 'cosmetic' suggestions that are too big/risky for the scope of the 2.4->2.6
> change. Given the indicated time constraints, I tried to really stick to
> modified code, though additional comments can be found in a separate section
> of my code review....
>
> Thanks,
> Keith
>
> AI_database.py:
>
> 301, 414: Alignment
Thank you
> 329: typo: noraml -> normal
(And 318)
I pulled normal out of the comment entirely as I couldn't figure
out what normal meant, versus simply decimal clarifying this.
> 314-331: Ridiculously trivial alignment nit, but I believe the subsequent
> lines should be adjusted such that the first character of the line is
> lined-up with the first non-parentheses character of the previous line.
Thank you! You're spot on here and this was completely an uncaught error
on my part.
> 394: "if strip:"
(367 should be not (onlyUsed or strip) too, 374 not onlyUsed, 421 and
humanOutput, 174 if self._committable, 193 request.needsCommit() and
self._committable, 306 provideManNameAndInstance:, 206 not
request.needsCommit(), )
Thank you for reminding me to check this!
> delete_manifest.py:
>
> 77, 103, 121: Remove spaces around the '=' in "commit = True"
(144, 45, 46, 47, 48 too)
Thank you for reminding me to grab these!
> 116: Alignment
Ah yes, if I break at "man_name," I can fit the next line within 80 cols.
> list-manifests.py:
>
> 46, 106: Alignment
Thank you
> 78, 86: Use of str.ljust (and/or str.rjust) would be simpler and more
> appropriate (here and in several changed spots below)
> http://docs.python.org/library/stdtypes.html#str.ljust
Nice, this is a more readable way to approach this. My question is would
this all still be better done with printf since printf style format
strings are so ubiquitous to so many?
I don't really see line 86 being that well served by this, as I would have
to do the odd "-".center(length, "-") which, to me, seems goofy. However,
you can see there's a number of other places this worked well.
> 175,177: No longer needed (change 181,182 to be = instead of +=)
Yup
> webserver.py:
>
> 77: Revert this, the original name was PEP8 accurate.
Ah yes, one of these days I'll internalize PEP8's naming conventions
> 122,123, 206, 353-356, 361: Remove spaces around '='
(And 284, 96, 166, 218, 309, 338 too)
Thank you
>
> -- Additional Comments --
>
> AI_database.py
>
> 32: This second import doesn't seem to get us much, can we remove it?
You're right this does seem frivolous.
> 82: nit: I find "if <x> not in <y>" to be more readable than "if not <x> in
> <y>"
Um, I don't think you mean line 82 but 80 (as 82 moved in the new
version)? I can appreciate your nit and have changed
> 175, 193, 206, 306, 367, 374, 394, 421: Use direct comparison (e.g., "if not
> <some_boolean>:" instead of comparing to True/False ("if <some_boolean> ==
> False:")
Ah yes, indeed!
> 377-378: Could just be
> if not column.startswith('MAX'):
> yield str(column.replace('MIN', ''))
Good collapse of the logic
> 399-409: Should be changed to use the 'if key in dict:' syntax
Good catch I must have been playing obscure Python when I wrote that...
I used:
if not response.haskey(<long
key>):
[...]
As this was more readable than:
if <long key>
not in response:
[...]
> delete_manifest.py:
>
> 74: "is None:"
Yup
> list-manifests.py:
>
> 198: "for cri in criteria:"
This needs to be criteria.keys() as this is an sqlite3 object which
does not behave as a dictionary.
> verifyXML.py:
>
> 119: Bare 'except' clause (Should also be raising a more 'relevant'
> ValueError here.
Yeah, this has been not so great. I fixed the IPv4 here (you should really
change this to use the Text Installer's class if you do one) and I've
substituted the MACAddress class in for the MAC address check. I have both
throwing a ValueError as you suggest for symmetry.
> webserver.py:
>
> 124: Could just be "for crit, value in critPairs.items():"
It could be, however, this is an sqlite3 object which doesn't support
items(). I've added a comment.
> 215, 289, 306, 316, 335: Can we use the @staticmethod decorator instead?
> Also, staticmethods should not have a 'self' parameter.
Yes; this was my first Python and mostly first OO program. This was
completely superfluious.
> 218, 284, 309, 338: Remove spaces around '='
Thank you, I think I've cleaned these.
> Clay Baenziger wrote:
>> Hi all,
>> I've updated the usr/src/cmd/ai-webserver components to be 2.6 clean.
>> Can Jean and/or Joe look at these changes and get responses back so that we
>> can wrap up by the 9th?
>> Further, in testing, I found a number of bugs which hadn't been
>> addressed but which fully broke these components and prevented me from
>> testing my changes -- I've as such, fixed these too. (I guess the
>> components don't see much use?) Unfortunately, there's not any regression
>> tests for publishing/listing/deleting manifests that I'm aware of. As such
>> I've done the following:
>>
>> Tests -- for manifest publication
>> (all should work unless otherwise specified):
>>
>> -------------------------------------------------------------------------------
>>
>> Try the webserver stock out of an fresh create-service
>> Try to download the default.xml file via the BUI
>> (All tests below involve checking list-manifest and the webserver BUI to
>> ensure things are correct afterwords)
>> Try to publish a new default manifest
>> Try to publish a new default manifest (but with criteria) -- should fail
>> Try to publish a new non-default manifest (with one criteria)
>> Try to publish a the same non-default manifest (but with two criteria)
>> Try to publish a new non-default manifest (with any number of criteria)
>> Try to publish a non-default manifest (with a duplicate criteria set) --
>> should fail
>> Try to download non-default manifest using manual testing interface
>> (http://<server>:<port>/manifest.html)
>> Try to remove an instance of the first manifest
>> (delete-manifest -i <instance> <manifest name> /var/ai/<port>)
>> Try to remove all instances of the first manifest
>> (installadm remove -n <service> -m <manifest>)
>>
>> PyLint:
>>
>> -------------------------------------------------------------------------------
>>
>> These files have seen some PEP8 clean-up, however, due to time constraints
>> not all PEP8 suggestions were done. The pylint scores using the
>> install.pylint rc file are as follows:
>> verifyXML.py
>> Your code has been rated at 5.84/10
>> AI_database.py
>> Your code has been rated at 3.39/10
>> publish-manifest.py
>> Your code has been rated at 3.12/10
>> delete-manifest.py
>> Your code has been rated at 1.17/10
>> webserver.py
>> Your code has been rated at 1.01/10
>> list-manifest.py
>> PyLint fails to run on this
>>
>> Webrev:
>> http://cr.opensolaris.org/~clayb/python2.6/
>>
>> Bugs addressed:
>> 8015 list-manifests output is hard to read
>> 11607 Should remove "prototype" out of AI commands
>> 12162 Need to transition the AI webserver to use the SQLite3 module
>> instead of PySQLite2
>> 12424 AI Websever unable to handle manifest with null IPv4 or MAC address
>> criteria if set in at least one manifest
>>
>> Thank you,
>> Clay
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
