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

[Rajeev S]

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!