Merge lp:~gz/bzrtools/shell_without_readline_719075 into lp:bzrtools

[Martin Packman]

Allows the readline import to fail in bzrtools.shell and modifies PromptCmd to tolerate the absence of the readline module. The autocompletion is explictly disabled if this is the case, it would work anyway, but it saves cmd.Cmd repeatedly trying and failing to import readline.

The test is a little black magic, but I think it should be reliable, and function on all platforms to ensure this doesn't accidentally regress.

[Aaron Bentley (abentley)]

Making readline optional is fine, but please retain PEP8 import ordering-- readline should still be imported between os and shlex, not as if it was a third-party library.

I don't understand why you're explicitly disabling autocompletion. On platforms without readline, it won't work, but I don't think that incurs overhead.

Your test is broken. It passes (on Ubuntu Maverick) even if I revert the changes to shell.py. I believe it is sensitive to differences in python path.

I don't think it's sensible to test interactive functionality using a blackbox test. Tests are not required for "shell". Actually, all of bzrtools is a bit lax.

review: Needs Fixing

[lp:~gz/bzrtools/shell_without_readline_719075]

758. By Martin Packman on 2011-02-16

Remove test as it can't fail reliably cross platform

759. By Martin Packman on 2011-02-16

Address review comments from Aaron Bentley

[Martin Packman (gz)]

> Making readline optional is fine, but please retain PEP8 import ordering--
> readline should still be imported between os and shlex, not as if it was a
> third-party library.

Okay, seemed a bit odd to mix plain imports with try/except.

> I don't understand why you're explicitly disabling autocompletion. On
> platforms without readline, it won't work, but I don't think that incurs
> overhead.

You're right. It's mostly documentary. Changed.

> Your test is broken. It passes (on Ubuntu Maverick) even if I revert the
> changes to shell.py. I believe it is sensitive to differences in python path.
>
> I don't think it's sensible to test interactive functionality using a blackbox
> test. Tests are not required for "shell". Actually, all of bzrtools is a bit
> lax.

Thanks for double checking that. Had forgotten some of the intricacies of sys.path wrt interactive mode vs running a script and platform/version differences.

I don't like that shell isn't really tested, but have removed that attempt as I'm not sure even messing with PYTHONPATH would be enough given setuptools and other complications.

=== modified file 'shell.py'
--- shell.py	2011-02-15 01:04:23 +0000
+++ shell.py	2011-02-16 01:10:13 +0000
@@ -18,7 +18,12 @@
 import cmd
 from itertools import chain
 import os
-import readline
+try:
+    import readline
+except ImportError:
+    _has_readline = False
+else:
+    _has_readline = True
 import shlex
 import stat
 import string
@@ -100,14 +105,16 @@
         ensure_config_dir_exists()
         self.history_file = osutils.pathjoin(config_dir(), 'shell-history')
         whitespace = ''.join(c for c in string.whitespace if c < chr(127))
-        readline.set_completer_delims(whitespace)
-        if os.access(self.history_file, os.R_OK) and \
-            os.path.isfile(self.history_file):
-            readline.read_history_file(self.history_file)
+        if _has_readline:
+            readline.set_completer_delims(whitespace)
+            if os.access(self.history_file, os.R_OK) and \
+                os.path.isfile(self.history_file):
+                readline.read_history_file(self.history_file)
         self.cwd = os.getcwd()
 
     def write_history(self):
-        readline.write_history_file(self.history_file)
+        if _has_readline:
+            readline.write_history_file(self.history_file)
 
     def do_quit(self, args):
         self.write_history()