OVH email manager script using docopt

Question

This is what I coded to realize a command line program to make it easier to manage emails on ovh shared hostings. It's working ok but it seems ugly.

I'm a beginner with Python and docopt. What would you suggest?

#!/usr/bin/env python
# -*- coding: utf-8 -*-
''' OVH Email Manager (ovhEmailMan)
A small script that helps to add and remove one or more email addresses on the OVH shared domains

    Usage:
        ovh_mails.py list [--ugly]
        ovh_mails.py add (<address> [--pswd=<password>][--description=<description>] | --file <filename> [--notify])
        ovh_mails.py remove (<address> | --file <filename>)
        ovh_mails.py (-h | --help)

    Arguments:
        <password>                        Password to access the mailbox (if not provided it's random generated)
        <filename>                        Name of the files to process (csv). Check README to see how to format it

    Options: 
        -h, --help                        Show this help message
        -u, --ugly                        Print without nice tables
        -p, --pswd=<password>             Set the password to the one provided
        -n, --notify                      If set, notification mail is sent using smtp credentials in ovh.conf

    Commands:
        list                              list all the email addresses currently configured
        add                               add one or more (configured in <filename>) email addresses
        remove                            remove one ore more (configured in <filename>) email addresses
    '''



import ovh
from docopt import docopt
from ovhem import EmailManager
from ovhem import fileprocesser as fp


if __name__ == '__main__':
    args = docopt(__doc__)
    #Validate args ---- TODO
    eman = EmailManager()

    # 'List' command parsing
    if args['list']:
        if args['--ugly']:
            eman.niceoutput = False

        eman.list_emails()
    # 'Add' command parsing
    elif args['add']:
        if args['<address>']:

            emails = (
                      {
                       'address': args['<address>'],
                       'password': None,
                       'description': None,
                       },
                      )
            if args['--description']:
                emails[0]['description'] = args['<description>']
            if args['--pswd']:
                emails[0]['password'] = args['<password>']
        if args['--file']:
            emails = fp.process_file(args['<filename>'])    

        # Getting back the emails dict
        emails=eman.add_emails(emails)
        if args['--notify']:
            fp.send_notifications(emails)

    # 'remove' command parsing       
    elif args['remove']:
        if args['<address>']:

            emails = (
                      {
                       'address': args['<address>'],
                       },
                      )
        if args['--file']:
            emails = fp.process_file(args['<filename>'])
        eman.remove_emails(emails)

Answer 1

Overall, it looks quite fine, with some smaller and bigger issues.

Parsing the add command

It's not so great to use the '<address>' string twice here:

    if args['<address>']:

        emails = (
                  {
                   'address': args['<address>'],
        # ...

You might mistype one of them. Or you might change something later and forget to update it everywhere. It would be better to put the value of args['<address>'] into a variable, and use that variable in evaluations and assignments.

This part is also awkward:

        emails = ( ... )
        if args['--description']:
            emails[0]['description'] = args['<description>']
        if args['--pswd']:
            emails[0]['password'] = args['<password>']

It would be much easier to use description and password variables to store defaults, then update these variables if --description or --pswd were given, and finally create the emails tuple.

Like this:

    address = args['<address>']
    if address:
        password = None
        description = None
        if args['--description']:
            description = args['<description>']
        if args['--pswd']:
            password = args['<password>']
        emails = (
            {
                'address': address,
                'password': password,
                'description': description,
            },
        )

Parsing the remove command

Here, if the second if statement is true, it will overwrite the effect of the first:

    if args['<address>']:
        emails = (
            {
                'address': args['<address>'],
            },
        )
    if args['--file']:
        emails = fp.process_file(args['<filename>'])

That suggest that you should use an elif, and switch the order of the statements:

    if args['--file']:
        emails = fp.process_file(args['<filename>'])
    elif args['<address>']:
        emails = (
            {
                'address': args['<address>'],
            },
        )

emails might be undefined

Both the parsing of add and remove have a problem: As their last step they do something with emails, for example eman.add_emails(emails) and eman.remove_emails(emails), but at that point the emails variable might be undefined.

This can be fixed by adding validation for these commands. It looks that either of these must be true, checked in this order:

• --file was specified and <filename> parameter was given
• <address> parameter was given

For example:

if args['--file']:
    filename = args['<filename>']
    if filename:
        emails = fp.process_file(filename)
    else:
        raise Exception("Filename parameter missing")
else:
    address = args['<address>']
    if address:
        emails = ( ... )
    else:
        raise Exception("Address parameter missing")

# main action