Code review comment for lp://qastaging/~gz/bzrtools/shell_without_readline_719075

Revision history for this message
Martin Packman (gz) wrote :

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

« Back to merge proposal