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

On 04.05.2010 12:32, Vincent Ladeuil wrote:
> For the sake of the discussion, imagine that we encounter a bug about an
> option with a weird character like ' or `, whatever.

The black-box tests are mostly about testing the fixed functionality
part of the template, not individual commands. But I've just adjusted
the get_script method to make it easier for future tests to provide
cooked completion data without actually having to provide commands for
these.

> Yeah, well, that's the point, I don't have strong opinions there as I
> don't know the code well enough to decide whether or not you can write
> the test in pythin or if bash is really required.

The bash completion stuff is advanced enough that a simplistic python
interpreter wouldn't suffice. And an afvanced one would be a project in
its own right, and require a full test suite to boot...

> Yes it is, look for shell-complete (not shell_complete) in contrib/zsh/_bzr.

OK, it does command name completion using that callback. But not option
completion, although that would be provided by the shell-completion
builtin as well, at least to some degree.

> 19 --- bzrlib/plugins/bash_completion/README.txt 1970-01-01 00:00:00 +0000
>
> This still includes material related to the use of the plugin in its
> non-core version.

Once the plugin got merged, I'd adjust the README to state the fact, and
point out that there are distinct lines of development. That much I'd
write for the standalone plugin and merge into core in a separate merge
request. So I'd like the README to stay a while, but get updated.

> Depending on how long this plugin will continue to live outside of the
> bzr tree, we may want to clean this stuff. I wonder how it will will
> behave (maintaining it in both trees and merging from the plugin may
> trigger some spurious conflicts whatever choise we make)...

I guess I'll keep the plugin around for about a year or so, so people
can use it even without updating their bzr setup. Once most distros ship
bzr with the plugin in place, I'll probably phase out the standalone
distribution. And in any case I'll probably not keep the standalone
branch up to date in case third parties provide branches against the bzr
tree affecting the plugin. Dunno how likely this is.

> Splitting the above into three assertCompletionXXXX will make the tests
> more explicit about what they are checking. Keeping the checks about the
> format returned may be left here though IMHO.

Done, and it does look better. Thanks!

> but 'Approved' with the tweaks mentioned above.

Why the plural? I'd consider the assertion stuff a single tweak, and the
rest of your comments I felt were suggestions, perhaps for a future
merge proposal, but in any case not requirements for this merge here.