> 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.
> 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.