Code review comment for lp:~reynolds.chin/glance/adaptive_index_listing

Revision history for this message
Kevin L. Mitchell (klmitch) wrote :

First, you should be aware that merging is now done through Gerrit; you should review the workflow at http://wiki.openstack.org/GerritWorkflow for information on that.

Now, on to your patch. My first major concern is that the constructs you use may not be 100% portable. I see that, in most places, you've used try/except blocks to try to mitigate that, but you use termios in getch() without a try/except, and termios is UNIX-specific. I also see some style problems—for instance, you removed a blank line (line 8), so there is only one line between the DEFAULT_COL_LIMIT assignment and the def catch_error(). Also, I think you assign to the wrong variable on line 34 of the patch, and your indentation in the blocks starting at line 40 seems off. The space before the comma on line 72 should probably be removed, and you don't have enough blank lines between def getch() and def user_response().

Have you run this through PEP8? That will highlight certain style problems I may not be seeing here. You should also run the test suite and make sure all the tests pass, particularly the functional tests that hit "glance index". (The test suite also, incidentally, runs PEP8; your patch has to get a clean bill of health from PEP8 before the automated system will allow it to be merged.)

« Back to merge proposal