Merge lp:~vila/django-configglue/be-verbose into lp:django-configglue

[Vincent Ladeuil]

Don't swallow standard django options like --verbosity.

In one project I'm working on which uses django_configglue, I came across
something weird, when running:

  django_project/manage.py test --noinput $APPS --verbosity=X

no matter which value I set X to, the output was the same.

Digging further, it happened that django_configglue was swallowing
--verbosity and the test command always received verbosity=1 (the default).

This patch fixes the issue by overriding GlueManagementUtility.execute() in
a narrower way.

I added a failing test prior to the fix and had to fix one other test as a
fallout (it's nice to have a test suite here that precisely caught them ;).

I tweaked another in test_settings for better output when it failed because
I had left the 'version=django.get_version()' when building the
LaxOptionParser (another nice catch of the test suite).

Now I can enjoy more verbosity when running my tests and I hope you will too ;)

[Ricardo Kirkner (ricardokirkner)]

l. 14: I'd use something like 'Override the base class to handle schema-related options.'
l. 24: is this really necessary? can't we just omit the param?
l. 65: depending on the branch taken by l. 25-34 either configglue_parser or parser is not defined
l. 64-68: this should only be invoked if configglue_parser is defined

given the simplified approach, maybe we can completely ditch the custom parser defined here (we're not really adding any value with it -- schemaconfigglue will create an empty parse if op is None -- or not passed in).

l. 84: no need to use a tuple for this case, but it's ok
l. 95-96: I'd use the standard patch('module.path') syntax to avoid having to import the module here
l. 101-107: as discussed on irc, with the simplified version, this test make no sense anymore (as all code paths will end up in a SystemExit, disregarding whether an exception was raised or not).
l. 117: we prefer singulars (assertEqual, not assertEquals)

review: Needs Fixing

[lp:~vila/django-configglue/be-verbose]

71. By Vincent Ladeuil on 2012-10-15

Take review comments into account and move the added test to clarify the resulting diff.

[Vincent Ladeuil (vila)]

> l. 14: I'd use something like 'Override the base class to handle schema-
> related options.'

Done.

> l. 24: is this really necessary? can't we just omit the param?

Done with better explanations in the comment.

> l. 65: depending on the branch taken by l. 25-34 either configglue_parser or
> parser is not defined
> l. 64-68: this should only be invoked if configglue_parser is defined

No. The diff makes it hard to realize but l. 65 is the else clause for the try so both configglue_parser and parser *are* defined

>
> given the simplified approach, maybe we can completely ditch the custom parser
> defined here (we're not really adding any value with it -- schemaconfigglue
> will create an empty parse if op is None -- or not passed in)

It seems that the LaxOptionParser is still needed there (I haven't dug fully to confirm why but *trying* to rely on the one provided by configglue has a weird side-effect on running the test suite: 6 tests are run instead of 50. I suspect the 'lax' to be required and probably a SystemExit raised by some test otherwise;)

And here we reach a funny paradox: if --verbosity was supported, the guilty test will be obvious, but if we don't use a lax parser, --verbosity is not recognized either :-D

So, I left the lax parser in for this proposal, may be another mp will be more appropriate to dig your suggestion ?

>
> l. 84: no need to use a tuple for this case, but it's ok

Right. It's prophylactic. When I know I will interpolate a single arg, I don't rely on python special casing the expected scalar-or-tuple-or-dict, I remove the ambiguity myself by always passing either a tuple or a dict. I've been bitten in the past by not doing so.

I left it as is, yell if you disagree with the rationale above ;)

> l. 95-96: I'd use the standard patch('module.path') syntax to avoid having to
> import the module here

Done.

> l. 101-107: as discussed on irc, with the simplified version, this test make
> no sense anymore (as all code paths will end up in a SystemExit, disregarding
> whether an exception was raised or not).

Test deleted.

> l. 117: we prefer singulars (assertEqual, not assertEquals)

Done.

[lp:~vila/django-configglue/be-verbose]

72. By Vincent Ladeuil on 2012-10-15

Not all equals are equal...

73. By Vincent Ladeuil on 2012-10-15

Simplify the logic, configglue use can be detected more directly.

[Ricardo Kirkner (ricardokirkner)]

Neat! Much cleaner

review: Approve

=== modified file 'django_configglue/management/__init__.py'
--- django_configglue/management/__init__.py	2012-01-13 15:08:52 +0000
+++ django_configglue/management/__init__.py	2012-10-15 14:32:20 +0000
@@ -67,58 +67,22 @@
 
 
 class GlueManagementUtility(ManagementUtility):
-    # This function was mostly taken from the django project.
-    # Please see the license file in the third-party/django directory.
     def execute(self):
-        """
-        Given the command-line arguments, this figures out which subcommand is
-        being run, creates a parser appropriate to that command, and runs it.
-        """
-        # Preprocess options to extract --settings and --pythonpath.
-        # These options could affect the commands that are available, so they
-        # must be processed early.
-        parser = LaxOptionParser(usage="%prog subcommand [options] [args]",
-                                 version=django.get_version(),
-                                 option_list=BaseCommand.option_list)
-        try:
-            configglue_parser = settings.__CONFIGGLUE_PARSER__
-            parser, options, args = schemaconfigglue(configglue_parser,
-                op=parser, argv=self.argv)
+        """Override the base class to handle the schema-related options. """
+        configglue_parser = getattr(settings, '__CONFIGGLUE_PARSER__', None)
+        if configglue_parser is not None:
+            # We need a lax option parser that:
+            # - allows the '%prog subcommand [options] [args]' format
+            # - will receive the schema options from configglue
+            # - doesn't attempt to recognize django options
+            lax_parser = LaxOptionParser(
+                usage="%prog subcommand [options] [args]")
+            parser, options, args = schemaconfigglue(
+                configglue_parser, op=lax_parser, argv=self.argv)
+            utils.update_settings(configglue_parser, settings)
             # remove schema-related options from the argv list
             self.argv = args
-            utils.update_settings(configglue_parser, settings)
-        except AttributeError:
-            # no __CONFIGGLUE_PARSER__ found, fall back to standard django
-            # options parsing
-            options, args = parser.parse_args(self.argv)
-            handle_default_options(options)
-        except:
-            # Ignore any option errors at this point.
-            args = self.argv
-
-        try:
-            subcommand = self.argv[1]
-        except IndexError:
-            sys.stderr.write("Type '%s help' for usage.\n" % self.prog_name)
-            sys.exit(1)
-
-        if subcommand == 'help':
-            if len(args) > 2:
-                self.fetch_command(args[2]).print_help(self.prog_name, args[2])
-            else:
-                parser.print_lax_help()
-                sys.stderr.write(self.main_help_text() + '\n')
-                sys.exit(1)
-        # Special-cases: We want 'django-admin.py --version' and
-        # 'django-admin.py --help' to work, for backwards compatibility.
-        elif self.argv[1:] == ['--version']:
-            # LaxOptionParser already takes care of printing the version.
-            pass
-        elif self.argv[1:] == ['--help']:
-            parser.print_lax_help()
-            sys.stderr.write(self.main_help_text() + '\n')
-        else:
-            self.fetch_command(subcommand).run_from_argv(self.argv)
+        super(GlueManagementUtility, self).execute()
 
 
 # We're going to go ahead and use our own ManagementUtility here, thank you.

=== modified file 'django_configglue/tests/test_configglue.py'
--- django_configglue/tests/test_configglue.py	2012-01-13 01:33:25 +0000
+++ django_configglue/tests/test_configglue.py	2012-10-15 14:32:20 +0000
@@ -352,8 +352,9 @@
     def test_execute_no_args(self):
         self.util.argv = ['']
         self.assertRaises(SystemExit, self.execute)
-        self.assertEqual(self.capture['stderr'],
-            "Type '%s help' for usage.\n" % self.util.prog_name)
+        self.assertIn(
+            "Type '%s help <subcommand>' for help" % (self.util.prog_name,),
+            self.capture['stderr'])
 
     def test_execute_help(self):
         self.util.argv = ['', 'help']
@@ -395,19 +396,20 @@
         finally:
             wrapped.__CONFIGGLUE_PARSER__ = old_CONFIGGLUE_PARSER
 
-    @patch('django_configglue.utils.update_settings')
-    def test_execute_configglue_exception(self, mock_update_settings):
-        mock_update_settings.side_effect = Exception()
-
-        self.util.argv = ['', 'help']
-        self.assertRaises(SystemExit, self.execute)
-        self.assertTrue(self.util.main_help_text() in self.capture['stderr'])
-
     def test_execute_with_schema_options(self):
         self.util.argv = ['', '--django_debug=False', 'help', 'settings']
         self.execute()
         self.assertTrue('Show settings attributes' in self.capture['stdout'])
 
+    def test_verbosity_is_preserved(self):
+        self.util.argv = ['', 'settings', '--verbosity=2']
+        handle_path = ('django_configglue.management.commands.settings.'
+                       'Command.handle')
+        with patch(handle_path) as mock_handle:
+            self.execute()
+            args, options = mock_handle.call_args
+            self.assertEqual('2', options['verbosity'])
+
 
 class LaxOptionParserTestCase(TestCase):
 

=== modified file 'django_configglue/tests/test_settings.py'
--- django_configglue/tests/test_settings.py	2011-09-16 23:10:48 +0000
+++ django_configglue/tests/test_settings.py	2012-10-15 14:32:20 +0000
@@ -212,7 +212,7 @@
         finally:
             self.end_capture()
         expected = get_version()
-        self.assertTrue(self.capture['stdout'].count(expected) == 1)
+        self.assertEqual(1, self.capture['stdout'].count(expected))
 
     def test_noargs_doesnt_error(self):
         args = ['manage.py']