[Mailman-Developers] [CLI Project] Added Backup and Restore Tool

[Stephen J. Turnbull]

Hi,

I have some comments on your CLI, r69.  File is attached.

Steve

-------------- next part --------------
Comments on Rajeev's CLI
========================

================== =====================================================
Comment Date:      2014-08-05 22:16
Referenced Commit: rajeevs1992 at gmail.com-20140804184004-kaan7063cifoasb1
Author:            Rajeev S <rajeevs1992 at gmail.com>
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.