HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

OVH email manager script using docopt

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptdocoptemailusingmanagerovh

Problem

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 ( [--pswd=][--description=] | --file [--notify])
ovh_mails.py remove ( | --file )
ovh_mails.py (-h | --help)

Arguments:
Password to access the mailbox (if not provided it's random generated)
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= 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 ) email addresses
remove remove one ore more (configured in ) 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['']:

emails = (
{
'address': args[''],
'pa

Solution

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

Parsing the add command

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

if args['']:

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


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[''] into a variable, and use that variable in evaluations and assignments.

This part is also awkward:

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


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['']
    if address:
        password = None
        description = None
        if args['--description']:
            description = args['']
        if args['--pswd']:
            password = args['']
        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['']:
        emails = (
            {
                'address': args[''],
            },
        )
    if args['--file']:
        emails = fp.process_file(args[''])


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

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


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 ` parameter was given



  • ` parameter was given



For example:

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

# main action

Code Snippets

if args['<address>']:

        emails = (
                  {
                   'address': args['<address>'],
        # ...
emails = ( ... )
        if args['--description']:
            emails[0]['description'] = args['<description>']
        if args['--pswd']:
            emails[0]['password'] = args['<password>']
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,
            },
        )
if args['<address>']:
        emails = (
            {
                'address': args['<address>'],
            },
        )
    if args['--file']:
        emails = fp.process_file(args['<filename>'])
if args['--file']:
        emails = fp.process_file(args['<filename>'])
    elif args['<address>']:
        emails = (
            {
                'address': args['<address>'],
            },
        )

Context

StackExchange Code Review Q#82784, answer score: 2

Revisions (0)

No revisions yet.