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

• rdbisme: Thanks for your kind help. I will refactor as you suggest.