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

>>>>> Martin von Gagern <email address hidden> writes:

    >> check whether bash is available, and skip if it isn't.
    >> define a feature in bzrlib.tests.features for that.

    > Have a test for bash, but it's in my own testing code, as I can't
    > imagine other tests requiring bash as well, and as I wanted to
    > include the test suite in the standalone distribution of the
    > plugin as well.

Perfect.

    > Maybe some general feature class testing for the existence of
    > arbitrary binaries, possibly taking the PATH environment variable
    > and platform conventions into account might be a good idea as
    > well. But that would be a different merge request, I think.

Very good idea, many plugins could certainly benefit from it.

    >> You may even want to add more functions in the template
    >> to have smaller tests.

    > I don't feel like clobbering the bash function namespace with too
    > many functions. I consider the completion tests to be pretty much
    > black box tests: input a list of words, output a list of
    > completions.

Hehe, yes, you got it right, I'm not a big fan of blackbox tests :)

For the sake of the discussion, imagine that we encounter a bug about an
option with a weird character like ' or `, whatever.

I'd like the ability to write a test for just the option related part
without having to rely on a command that will use this option.

But except for the remarks bwlow, I'm fine with the tests you've added.

    > Otherwise I'd have to write not only a number of smaller
    > functions, but a suitable testing framework for all of these. Or
    > express test input and assertions in bash syntax.

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.

    >> bzrlib.builtins.cmd_shell_complete which is used by zsh (I think).

    > At least not by the zsh completion script shipped by either zsh or
    > bzr.

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

    > There are scripts out there making use of it, but none seem
    > particularly "official". In any case, I'll investigate zsh in more
    > detail once this got landed.

Yeah, no problem.

    >> Don't use relative imports, they tend to break in obscure ways

    > How so?

I don't remember the bug number off-hand, but having '.' in your
PYTHONPATH and '.' containing an unrelated python module with the same
name may wrongly trigger an import while using
'bzrlib.plugins.<plugin_name>' ensures we get the right one.

<snip/>

   > Don't want to do the same for the standalone distribution, though,
   > as there people might choose a different plugin name.

Wow, interesting... There are so many cases where you *need* the plugin
name to be the same as its containing directory (BZR_PLUGINS_AT works
hard to remove this limitation) that I didn't think about this case... I
won't explore it myself :)

Now for some specifics:

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.

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

No need to address that in the context of this proposal, I think people
will realize why the unrelated bits are there.

823 + def complete(self, words, cword=-1, expect=None,
824 + contains=[], omits=[]):

I think you try to do too much here:
864 + if expect is not None:
865 + self.assertEqual(set(expect), res)
866 + missing = set(contains) - res
867 + if missing:
868 + raise AssertionError('Completion should contain %r but it has %r'
869 + % (missing, res))
870 + surplus = set(omits) & res
871 + if surplus:
872 + raise AssertionError('Completion should omit %r but it has %r'
873 + % (surplus, res))

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.

So,
  review: needs_fixing

but 'Approved' with the tweaks mentioned above.