[PATCH v4 1/2] parsemail: Convert to a management command

[Andrew Donnellan]

On 04/09/16 10:12, Stephen Finucane wrote:
> From: Stephen Finucane <stephen.finucane at intel.com>

git commit --reset-author time? :) (S-O-B too.)

> Management comands allow applications to register their own actions with
> 'manage.py'. This provides some advantages, like automatically
> configuring Django (removing the need for 'django.setup' calls) and
> removing the need to set the PYTHON_PATH. The 'parsemail' script is a
> natural fit for this type of application. Migrate 'parsemail' to a
> management command.
>
> This includes some extensive work on logging configuration, as logging
> is moved from code into settings. In addition, it removes a lot of the
> customizable logging previously introduced in the parsemail command, in
> favour of modifications to the settings files.

Excellent, this simplifies things nicely!

> diff --git a/patchwork/management/commands/parsemail.py b/patchwork/management/commands/parsemail.py
> new file mode 100644
> index 0000000..65525ad
> --- /dev/null
> +++ b/patchwork/management/commands/parsemail.py
> @@ -0,0 +1,81 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2016 Intel Corporation
> +#
> +# This file is part of the Patchwork package.
> +#
> +# Patchwork is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# Patchwork is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Patchwork; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +import argparse
> +from email import message_from_file
> +import logging
> +from optparse import make_option
> +import sys
> +
> +import django
> +from django.core.management import base
> +
> +from patchwork.parser import parse_mail
> +
> +logger = logging.getLogger(__name__)
> +
> +
> +class Command(base.BaseCommand):
> +    help = 'Parse an mbox file and store any patch/comment found.'
> +
> +    if django.VERSION < (1, 8):
> +        args = '<infile>'
> +        option_list = base.BaseCommand.option_list + (
> +            make_option(
> +                '--list-id',
> +                help='mailing list ID. If not supplied, this will be '
> +                'extracted from the mail headers.'),
> +        )
> +    else:
> +        def add_arguments(self, parser):
> +            parser.add_argument(
> +                'infile',
> +                nargs='?',
> +                type=argparse.FileType('r'),
> +                default=sys.stdin,
> +                help='input mbox file (a filename or stdin)')
> +            parser.add_argument(
> +                '--list-id',
> +                action='store_true',

No parameters? This looks wrong...

> +                help='mailing list ID. If not supplied, this will be '
> +                'extracted from the mail headers.')
> +
> +    def handle(self, *args, **options):
> +        path = (args[0] if args else
> +                options['infile'] if 'infile' in options else None)

argparse is giving you a file, not a string. As you've given a default 
in the argument, this should always return an open file, I'm pretty sure.

> +        stdin = options.get('stdin', sys.stdin)

What's this for?

> +
> +        # Attempt to parse the path if provided, and fallback to stdin if not
> +        if path and not isinstance(path, file):
> +            logger.info('Parsing mail loaded by filename')
> +            with open(path, 'r+') as file_:
> +                mail = message_from_file(file_)
> +        else:
> +            logger.info('Parsing mail loaded from stdin')
> +            mail = message_from_file(stdin)
> +

As noted in my other email this is all broken.

I'd rewrite all this as follows (lightly tested on Py2 and 3):

     def handle(self, *args, **options):
        infile = options['infile']
        if infile == sys.stdin:
            logger.info('Parsing mail loaded from stdin')
        else:
            logger.info('Parsing mail loaded by filename')
        mail = message_from_file(infile)

> diff --git a/patchwork/tests/test_management.py b/patchwork/tests/test_management.py
> new file mode 100644
> index 0000000..a3f4720
> --- /dev/null
> +++ b/patchwork/tests/test_management.py
> @@ -0,0 +1,51 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2016 Stephen Finucane <stephenfinucane at hotmail.com>
> +#
> +# This file is part of the Patchwork package.
> +#
> +# Patchwork is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# Patchwork is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Patchwork; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +import os
> +
> +from django.core.management import call_command
> +from django.test import TestCase
> +from django.utils.six import StringIO
> +
> +from patchwork.tests import TEST_MAIL_DIR
> +
> +
> +class ParsemailTest(TestCase):
> +    def test_invalid_path(self):
> +        with self.assertRaises(IOError):
> +            call_command('parsemail', 'xyz123random')
> +
> +    def test_path_failure(self):
> +        # we haven't created a project yet, so this will fail
> +        with self.assertRaises(SystemExit) as exc:
> +            call_command('parsemail',
> +                         os.path.join(TEST_MAIL_DIR,
> +                                      '0001-git-pull-request.mbox'))
> +
> +        self.assertEqual(exc.exception.code, 1)
> +
> +    def test_stdin_failure(self):
> +        # we haven't created a project yet, so this will fail
> +        with open(os.path.join(TEST_MAIL_DIR,
> +                               '0001-git-pull-request.mbox')) as file_:
> +            with self.assertRaises(SystemExit) as exc:
> +                call_command('parsemail',
> +                             stdin=file_)
> +
> +            self.assertEqual(exc.exception.code, 1)

Can we test the --list-id option at all?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited