Comments on Rajeev's CLI ======================== ================== ===================================================== Comment Date: 2014-08-05 22:16 Referenced Commit: rajeevs1992@gmail.com-20140804184004-kaan7063cifoasb1 Author: Rajeev S Branch Nick: mailmancli Commit Date: Tue 2014-08-05 00:10:04 +0530 ================== ===================================================== Tree organization ----------------- I don't think the CLI should be buried several levels deep in the source. The mailman.client package is really a collection of services, and the CLI should be at the same level in some sense. All file references below are relative to mailman.client/src/mailmanclient/cli. Is there any code outside of this directory that is yours? It's hard to recognize which files are part of this project. This should be reflected in header comments and/or module docstrings. I think that these files should also acknowledge support from Google, and provide contact addresses (yours and mentors'). There should be a README file of some kind at the top of your subtree, or perhaps a section in the mailman.client top level README. mmclient.py ----------- This file is part of your 2014 GSoC project, and not inherited from mailman.client, right? I think there should be some comments in this file, for example explaining why you have what seem to be two different REPLs (c.run() and s.cmdloop()) invoked. It's not clear to me why you define a separate main() function in this script -- it doesn't make sense to import it as a module. docs ---- Run a spellchecker on these files (I saw at least one typo). Generally they look very good: clear and complete. lib --- In lib/utils.py, I think there should be a more flexible way to specify the emphasis and so on. This is especially important for color-blind and partially-blind users. For now you don't need to provide a user interface, but I'd be happier if the control sequences were kept in module-level variables and interpolated into the messages using %-formatting or str.format. (Note that if you wanted to, you could use HTML tags in such variables. Eg, many Qt4 and Qt5 widgets that can display text respect HTML tags in display.) In lib/utils.py class Filter, what are the class attributes for? They are not used or useful. core ---- In core/domains.py, you instantiate the Domain list then delete it, commenting it as testing the connection. Why does this need to be done in the constructor? Although Domain doesn't currently provide any services that can be accomplished without a database query that I noticed, so the check doesn't really hurt, it could. (For example, validating domain names against the syntax recommended by RFCs 822 and 1034 for mail domains.) The same kind of service would apply to list names. It's also possible that there is a transient interruption that is resolved before any actual work is done, but this check would make the script fail. BTW, I believe there is no need to assign self.client.domains to a variable (the Python optimizer isn't that smart AFAIK). Even if you do assign, there's no need to delete the variable, as the value will be GC'd after exiting the constructor. IIRC, del doesn't guarantee that the object will be GC'd any time soon, just DECREF's it. Assuming that an HTTPError means that the operation failed in an expected way (eg, deleting a nonexistent domain) is unacceptable. In particular, the transaction may fail if Mailman is stopped. The code must check the information provided by the HTTPError object, at least for errors that can be expected from invalid input. The rest can be "Unknown HTTPError was raised" for now. The various classes have a lot of code in common (eg, in the __init__() and get_listing() methods). I don't know if it's worth refactoring because there is a lot of class-specific code in these "lookalike" methods, but there does seem to be some potential for it. client ------ I don't understand the parser yet, but the rest of the code looks clean. tests ----- The tests look complete and properly set up.