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 329: typo: noraml -> normal 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. 394: "if strip:" delete_manifest.py: 77, 103, 121: Remove spaces around the '=' in "commit = True" 116: Alignment list-manifests.py: 46, 106: Alignment 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 175,177: No longer needed (change 181,182 to be = instead of +=) webserver.py: 77: Revert this, the original name was PEP8 accurate. 122,123, 206, 353-356, 361: Remove spaces around '=' -- Additional Comments -- AI_database.py 32: This second import doesn't seem to get us much, can we remove it? 82: nit: I find "if <x> not in <y>" to be more readable than "if not <x> in <y>" 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:") 377-378: Could just be if not column.startswith('MAX'): yield str(column.replace('MIN', '')) 399-409: Should be changed to use the 'if key in dict:' syntax delete_manifest.py: 74: "is None:" list-manifests.py: 198: "for cri in criteria:" verifyXML.py: 119: Bare 'except' clause (Should also be raising a more 'relevant' ValueError here. webserver.py: 124: Could just be "for crit, value in critPairs.items():" 215, 289, 306, 316, 335: Can we use the @staticmethod decorator instead? Also, staticmethods should not have a 'self' parameter. 218, 284, 309, 338: Remove spaces around '=' 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
