Code review comment for lp:~gagern/bzr/bug560030-include-bash-completion-plugin

Revision history for this message
Vincent Ladeuil (vila) wrote :

I'm ok with merging this into core, but I'd like a few cleanups first.

Mostly, the plugin carries some excessive baggage from its birth as a standalone plugin that doesn't match the policies we have for the core plugins.

Regarding distribution, windows installers need to be told explicitly about new
plugins, so no worries there. For the other OSes, well, bash is at least available
if not already installed by default.

Regarding load time impact, you can go a step further and use CommandRegistry.register_lazy().

We need tests. I don't clearly understand how the completion works and you may need to redesign a bit to be able to test at the python level. Don't hesitate
to ask for help.

I don't ask for a complete coverage here, but making sure we know how to
complete at least some basic commands and their args will be a minimum.
Having a design (with associated tests) ready for further enhancements
(prefix completion for known transport protocols or useful shortcuts like :parent, :push and the like) will be a nice bonus though.

The above are rather generic, here are some more concrete points:
- delete .bzrignore,
- use bzrlib version like the other plugins,
- watch for PEP8 compliance (head, fun, tail, etc in bashcomp.py miss double vertical spaces)

review: Needs Fixing

« Back to merge proposal