Hi Steve,
On Wednesday 06 August 2014 12:14 PM, Stephen J. Turnbull wrote:
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.
I feel that it will make the code cluttered. Since the CLI code is independent of the rest of mailman client, won't it be better to maintain the CLI code in a separate folder, as it is now?
All file references below are relative to mailman.client/src/mailmanclient/cli.
Is there any code outside of this directory that is yours?
I have made modifications to setup.py, other than that, no.
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.
Will add those information in the headers by next revision.
mmclient.py
This file is part of your 2014 GSoC project, and not inherited from mailman.client, right?
Yes. I created this file.
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.
I was not quite familiar with using the python setuptools and I assumed that such a format was necessary to specify module entry points in setup.py. I have now removed the main function from mmclient.py
docs
Run a spellchecker on these files (I saw at least one typo).
Yikes! Sorry about that. Fixed at least ten of them :)
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.)
Sure. Will add that in the next revision.
In lib/utils.py class Filter, what are the class attributes for? They are not used or useful.
The Filter class is used to filter out objects meeting a certain condition specified by a key, op [aka operator] and value. The filtering process works by iterating through each element in the data_set and removing the objects that do not match the criterion, from a copy of the data_set.
The data_set cannot be used alone, as removing entries from a running
for i in list loop can
cause the loop to misbehave.
The filtered copy is finally returned.
All the class attributes are being used in the process described above.
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.
Now that the connection part is managed by a single function (Utils.connect), I will move the connection checking part to that function. I will try to come up with a better way to check for the connection, so that the mentioned possible issues can be put away.
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.
Those lists were not stored into variables until some revisions back. I got a random thought that it is a bad practice , and replaced that code with a variable assignment. It was then when my PEP8 checker returned an error stating the presence of an unused variable, which I solved by deleting the assigned variable!
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.
Will do.
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.
I will look into the possibility of refactoring in those parts, I have some plans in mind. Hope that those work!
client
I don't understand the parser yet, but the rest of the code looks clean.
I will add the grammar for each command to a README file under parsers, so that it can be studied, modified or extended with ease.
tests
The tests look complete and properly set up.
Will be adding more tests shortly.
All the mentioned changes (except the tests) would be completed at the latest, by 9th Aug 2014, Saturday. (r 70) Planning to spend the weekend on writing new unit tests. (r 71)
Thank You!