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

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

    > Thanks for the review, Vincent! Probably won't have time to deal
    > with it before 27 April or so, so I'm putting this on hold until
    > then.

    > Testing is a real problem here: the things I'd be most concerned
    > about or interested in is bash syntax and behaviour, and obviously
    > testing these involves executing bash, which can't be done from
    > within python only. Maybe I can write tests that check whether
    > bash is available, and skip if it isn't.

Yup, that's the idea, define a feature in bzrlib.tests.features for
that.

    > I think this approach is the only thing that could really catch
    > regressions, in particular the one I recently introduced and fixed
    > on trunk. This is the only thing that can actually check the
    > behaviour of the hardcoded template around the simple dynamically
    > generated code.

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

    > Another thing I might do is refactor stuff so that data aquisition
    > and code generation are separated. That way, we could ensure that
    > the data aquisition does the correct thing, and hopefully keep the
    > code generation simple enough so it will be easy to check.

... no comment :)

    > This might also open the door for other shells, zsh in particular,
    > so it might be a good thing even without tests. But it's a major
    > step, so it will take a bit of time.

Yes, I didn't mention that in my first review and our experience is that
it's easier to review and land smaller proposals, so don't try to
address all of that in a single proposal.

But once you get there, have a look at
bzrlib.builtins.cmd_shell_complete which is used by zsh (I think).

Some more nits found while re-reading your mp:

- Don't use relative imports, they tend to break in obscure ways if you
  happend to have a PYTHON_PATH that includes the current dir and you're
  working in the directory of your plugin (or another plugin that use
  the same package name or another python program, etc). Even if *you*
  don't do that, we had bug reports in the past about that.

- try to use:

    from bzrlib import commands
    class cmd_foo(commands.Command)

  instead of

    from bzrlib.command import Command
    class cmd_foo(Command)

  We don't always respect this rule, but we're fixing such usages as we
  encounter them.