Merge lp:~jtv/maas/bug-1052876 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Print more helpful error message when maas-cli is run without arguments.

As discussed very briefly on the daily call. I make the invocation fail with error code 2 because that's what it did previously.

Jeroen

[Raphaël Badin (rvb)]

How about sparing the user an extra step and print the 'usage' string directly instead of message asking the user to re-run the command with -h (same as what, say, 'nmap' does)?

[Gavin Panella (allenap)]

[1]

+    output_file = '/dev/null'

'/dev/null' is defined as os.devnull too. I doubt /dev/null will
change in the next 10 years, but it may be microscopically less
magic to use os.devnull.

[2]

+        try:
+            self.run_command()
+        except CalledProcessError:
+            pass

You're expecting the command to exit with 2, so the following might be
more appropriate:

        error = self.assertRaises(CalledProcessError, self.run_command)
        self.assertEqual(2, error.returncode)

In fact, if run_command() were changed, this test could be simplified
further without affecting other tests:

    from subprocess import check_output, STDOUT

    def run_command(self, *args):
         return check_output(
             [locate_maascli()] + list(args),
             stderr=STDOUT)

    def test_run_without_args_shows_help_reminder(self):
        error = self.assertRaises(CalledProcessError, self.run_command)
        self.assertEqual(2, error.returncode)
        self.assertIn(
            "Run %s --help for usage details." % locate_maascli(),
            error.output)

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. Taking the error from the exception simplifies things nicely. On the other hand, I really don't care what the return code is, as long as it's not zero! No need to make tests depend on that, I think.

Jeroen

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Gavin Panella (allenap)]

[3]

+    def test_run_without_args_shows_help_reminder(self):
+        self.output_file = self.make_file('output')
+        try:
+            self.run_command()
+        except CalledProcessError as e:
+            pass
+        self.assertIn(
+            "Run %s --help for usage details." % locate_maascli(),
+            e.output)

This should use assertRaises(). If run_command() does not raise any
exception, this test will break with `NameError: name 'e' is not
defined`.

[4]

Don't forget rvba's question:

> How about sparing the user an extra step and print the 'usage'
> string directly instead of message asking the user to re-run the
> command with -h (same as what, say, 'nmap' does)?

[Julian Edwards (julian-edwards)]

On 21/11/12 00:08, Gavin Panella wrote:
> [3]
>
> + def test_run_without_args_shows_help_reminder(self):
> + self.output_file = self.make_file('output')
> + try:
> + self.run_command()
> + except CalledProcessError as e:
> + pass
> + self.assertIn(
> + "Run %s --help for usage details." % locate_maascli(),
> + e.output)
>
> This should use assertRaises(). If run_command() does not raise any
> exception, this test will break with `NameError: name 'e' is not
> defined`.

Also remember that assertRaises() returns the exception, so you can
further assert the exception text if you want.

[Jeroen T. Vermeulen (jtv)]

Sorry for forgetting about that other question: why not print out usage info if no options are passed? This is getting into taste, where mine is to separate user-doesn't-know-what-they're-doing from user-wants-usage-information. It can be annoying to someone who's not familiar with the Unix-style command line at all, but it does make some sense and it is established in Unix tradition.

Command-line users probably won't run a command without arguments to find out how it works: we have the -h convention and manpages for that. But they might run it without arguments for other reasons. They expect it to do something else (interactive MAAS CLI?) or they might do it by accident (a script passes $ARGS but there's nothing in ARGS). In those cases, the important message is “this is not how it works” and we shouldn't be drowning that out with usage details.

[Raphaël Badin (rvb)]

Thanks for the reply.

> It can be annoying to someone who's not familiar with the Unix-style command line at all, but it does make some sense > and it is established in Unix tradition.
[...]

Maybe it's a matter of taste but for complex cli tools with a rich feature set (think nmap, or sudo maas [i.e. the django cli]), I think it's best to show the help directly because (at least it's what I do), when you've forgotten about the usage, you just type the command and get the nice help text to refresh your memory. Of course, it's ok to think about adding '-h' but I don't see any reason to impose that extra step.

> But they might run it without arguments for other reasons. They expect it to do something else (interactive MAAS CLI?) > or they might do it by accident (a script passes $ARGS but there's nothing in ARGS). In those cases, the important
> message is “this is not how it works” and we shouldn't be drowning that out with usage details.

If there is a human on the other side, then it does not hurt to show the help directly. If this is done by a script, then the non-zero return code is all that matters.

But anyway, it's really a detail.

[Gavin Panella (allenap)]

Fwiw, I'm with jtv on this. Having the full help printed is useful the
first 3 or 4 times I incorrectly invoke the command, or after a period
of months without using it, perhaps, but after that it's a nuisance,
especially when I suddenly get 30-40 lines of text because of a typo,
and my context has scrolled off the screen. I'd rather live with a
terse response and having to add -h.

[Julian Edwards (julian-edwards)]

On 23/11/12 02:22, Gavin Panella wrote:
> Fwiw, I'm with jtv on this. Having the full help printed is useful the
> first 3 or 4 times I incorrectly invoke the command, or after a period
> of months without using it, perhaps, but after that it's a nuisance,
> especially when I suddenly get 30-40 lines of text because of a typo,
> and my context has scrolled off the screen. I'd rather live with a
> terse response and having to add -h.
>

I have the exact same thought. I did initially think what rvb says but
Gavin nailed it - context scrolling off the top of your terminal is
*really* annoying.

Cheers

=== modified file 'src/maascli/__init__.py'
--- src/maascli/__init__.py	2012-10-08 14:07:24 +0000
+++ src/maascli/__init__.py	2012-11-19 16:14:20 +0000
@@ -28,6 +28,14 @@
     if argv is None:
         argv = sys.argv[:1] + osutils.get_unicode_argv()
 
+    if len(argv) == 1:
+        # No arguments passed.  Be helpful and point out the --help option.
+        sys.stderr.write(
+            "Error: no arguments given.\n"
+            "Run %s --help for usage details.\n"
+            % argv[0])
+        raise SystemExit(2)
+
     parser = prepare_parser(argv)
 
     # Run, doing polite things with exceptions.

=== modified file 'src/maascli/tests/test_integration.py'
--- src/maascli/tests/test_integration.py	2012-10-04 08:36:34 +0000
+++ src/maascli/tests/test_integration.py	2012-11-19 16:14:20 +0000
@@ -15,7 +15,8 @@
 import os.path
 from subprocess import (
     CalledProcessError,
-    check_call,
+    check_output,
+    STDOUT,
     )
 
 from maastesting.testcase import TestCase
@@ -34,14 +35,21 @@
 class TestMAASCli(TestCase):
 
     def run_command(self, *args):
-        with open('/dev/null', 'ab') as dev_null:
-            check_call(
-                [locate_maascli()] + list(args),
-                stdout=dev_null, stderr=dev_null)
+        check_output([locate_maascli()] + list(args), stderr=STDOUT)
 
     def test_run_without_args_fails(self):
         self.assertRaises(CalledProcessError, self.run_command)
 
+    def test_run_without_args_shows_help_reminder(self):
+        self.output_file = self.make_file('output')
+        try:
+            self.run_command()
+        except CalledProcessError as e:
+            pass
+        self.assertIn(
+            "Run %s --help for usage details." % locate_maascli(),
+            e.output)
+
     def test_help_option_succeeds(self):
         self.run_command('-h')
         # The test is that we get here without error.