On Friday 09 May 2014 11:12 PM, Stephen J. Turnbull wrote:
I would like to post this to Mailman-Developers, with your permission.
Rajeev S writes:
Hi Steve,Can I go forward with the current design?
Looking at the code you've produced so far[1] I can say it's clean and (mostly) Pythonic, easy to read, making good use of packaged modules (and good choices of the ones to use).
But to be honest, you haven't really *presented* a design yet. A file tree as in your blog[2] isn't a design, and the descriptions of commands really amount to a set of example use cases, but hardly complete. The design is implicit in patterns in the examples, and I have a feeling that it isn't yet coherent in your mind. I only meant to expose my approach towards the project. Now, at least you know how I have planned to approach the task at hand. So you can go forward, but if you continue to write code "as you go along", I think every couple of weeks we're going to have to refactor. That's OK if that's what you want (as you say in your blog "discussions over a piece of code would be more effective than just talk"). The alternative is to systematically explore the space of command + argument + option combinations, and try to come up with a language that compactly expresses the most common operations. I recommend this approach if it works for you, because various options are starting to proliferate, and I would rather we keep them to a minimum so that users don't need to remember them all or be continuously consulting the documentation. It may *not* work for you, and that's OK, we can go with the refactor-until-happy strategy then. I too would prefer to go by the systematic approach. I felt the best way to express my thoughts was to implement some part of it. I am refactoring too much already. A couple of examples of the kind of things I think it would be useful to think about in advance:
(1) You already have --domainname and --listname options, and of course the pattern suggests --username for sure, and maybe there will be additional --foonames. I wonder if it wouldn't be better to have a single --name option so that instead of
mmclient list create --domainname list.example.com --listname foo the user types mmclient list create --name foo --in-domain list.example.com or mmclient list create --namefoo@list.example.comI doubt the usability of a common option
name. As per the above snippet, you have used ain-domainto specify the domain name. There are many such instances where you would have to use more than onename, for instance adding moderators for a list. In such cases, options likedomainname,listnameandusernamewould be more intuitive than using options likein-domain.
Further, the option name is ambiguous for a user.
or even mmclient list createfoo@list.example.com Note that the last is "Pythonic" in the sense that your ArgumentParser will check for the right number of arguments.
This is a good way to do it, a more pythonic way,as mentioned.
Imposing a fair enough rule like the list must be specified only in
list@domain format can reduce a number of arguments in a *lot*
of commands.
By using a third (optional) positional argument that specifies
the name of the instance being managed, the commands look
more cleaner.
However, some commands still need more names to appear on the command
which I prefer *not* to replace by a single --name. The commands are
mentioned below.
(2) Should the scope (in [domain, list, user]) come first as in your current design, "mmclient list create", or should the action come first as in English, "mmclient create list"? I think the latter will be easier for non-technical admins to get used to, but it may not be -- we need a list of "things we want to say" to decide that. Commands that makes sense in English would suit most situations, in fact, all situations in current list of use cases.
As the first step of the systematic design process, I present the list of things we would want to say and how the command must look like and what possible arguments it should accept.
I have modified the CLI to use English like commands and hence will use them hereafter.
*list*
The command lists the entities and should be available for users,mailing lists and domains.
mmclient list list [list@domain.org] [-v for verbose] mmclient list user [list@domain.org] [-v for verbose] [--role ROLE] mmclient list domain [domain.org] [-v for verbose]
The ROLE would be any of member,owner or moderator.
*create*
Used to create an instance and should be available for domains, lists and users.
mmclient create domain domain.org [--contact CONTACT_EMAIL] mmclient create list list@domain.org mmclient create user foo@bar.com --name DISPLAY_NAME --password PASSWORD
*delete*
Removes a domain, list or user.
mmclient delete domain domain.org mmclient delete list list@domain.org mmclient delete user foo@bar.com
*Role management*
User roles can be managed by two actions, addrole and removerole, rather than 6 separate actions for subscribe, unsubscribe, addowner, addmoderator, removeowner, removemoderator
mmclient addrole user foo@bar.com --list list@domain.org --role <member,owner,moderator> mmclient removerole user foo@bar.com --list list@domain.org --role <member,owner,moderator>
The above commands are an argument longer than the commands like
mmclient addmoderator list list@domain.org --user foo@bar.com
but I feel former approach better, as it looks better and reduces the number of commands to learn/remember.
*Preferences/settings*
Settings would form a new scope, commands for which can be implemented
in a fairly straightforward way.
mmclient set setting <setting name> --value <value>
*Edit (user)*
Changing user credentials like email, password and display name
mmclient edit user foo@bar.com --email foo1@bar.com mmclient edit user foo@bar.com --name foo mmclient edit user foo@bar.com --password bar
A common method that will support multiple arguments together can be built for this action.
I feel that it's likely to be quite annoying to refactor a ton of code if we decide to change those aspects of the design. That's why I feel it's better to do a more detailed design now.
A few unconnected comments on various aspects:
- In your code you have some short variable names (d, l). This is mildly un-Pythonic. Try reading diffs (eg, cmdparser.py in r52) and I think you'll see. If you don't see right off, I can explain precisely why I stumbled in a couple of places, but I'd rather let you see it for yourself. Fixed.
- Short names for long options like --ll are a definite no-no. They make the code harder to understand, and on the command line they are arcane. We need to cater to users who may not be so flexible. I have replaced --ll with -v and --verbose.
- The change you make in r52 is interesting. I'm a little leery of the use of getattr(), it's ugly, but that's probably easily fixed.
- Once you have the parsed command, you look up the command and then apply it to arguments retrieved from your ArgumentParser instance. *These lookups can fail*, and you need to catch them (for interactive shell mode). I don't think ArgumentParser will check that all syntactically required arguments are present, so you need to do it yourself. Think carefully about this -- you may want to revise or even revert the change in r52 because it implies doing this argument checking in each action rather than in the command parser. Why not leave the argument verification to the action method itself? I feel that it's better to leave it there as it would make future addition of actions easier.
All the methods in the current version have a initial argument checking
part. The
current version already prints error messages if required arguments
are absent.
I will be replacing it with an exception so that it would be easier to
integrate
it with the interactive shell.
Also argparse returns a dictionary with keys for *all* the possible
options, whether or
not they are specified by the user. If the value of option is not
specified by the user,
then the value of corresponding key would be None.
However, extra arguments are ignored without warning.
- For detailed listings you add header in get_listing. But I think you should have a no-header option -- if you ever want to pipe that output to a script the header is annoying. My instinct was to move it out of that function, but I guess you have to do it there to get columnization right.
Some nitpicking about names:
"Operate" is pretty general, the conventional word for managing lists of things is "Manage".
"Long list" is ambiguous ("one 'long list'" vs. a "paged list", for example). "Detailed" (your word) or maybe "verbose" (conventional) is better.
I prefer the word "scope" for [domain, list, user] rather than "type" or "instance". Fixed.
Having "list" be both the name of a scope and the name of a command in the same ArgumentParser means that you need to check syntax (order of the words), not just presence of the words, to be sure the user has given all the required arguments. If the scope names and command names are disjoint, then you could even let the user choose whether to type "create list" or "list create". (That may not be a great idea.) The so called positional arguments are parsed in the order that
Adding --no-header requires just a trivial change.Will do.
they are listed in the initialize_options method. Hence the
order checking is performed by the argparse itself.
create domain is not same as domain create.
The presence of all required arguments is verified at the method corresponding to an action.
Footnotes: [1]http://bazaar.launchpad.net/~rajeevs1992/mailman.client/mailmancli/changes/
[2]http://myfossblog.blogspot.in/2014/05/lets-talk-over-cup-of-code.html
In addition, I will be partitioning the commands/arguments to groups based on the entity, by making use of the argparse sub-parsers. This would enhance the readability of the help strings.
TODO list from this mail:
- no-header option for list
- positional argument for instance name
- exceptions on argument lookup failure
- printing error messages on lookup failure
- partitioning arguments using sub-parsers
All parts mentioned fixed are fixed and will form r53, after some more cleaning, like the docs, probably by tomorrow.The items mentioned in TODO lists will be built once the design process is complete.