Code review comment for lp:~gagern/bzr/bug560030-include-bash-completion-plugin
Revision history for this message
| Martin von Gagern (gagern) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
> delete .bzrignore,
Dropped.
> use bzrlib version like the other plugins,
Done, and also dropped meta.py along the way.
> watch for PEP8 compliance
Should be better now.
> check whether bash is available, and skip if it isn't.
> define a feature in bzrlib.
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.
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.
> 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. 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.
> bzrlib.
At least not by the zsh completion script shipped by either zsh or 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.
> Don't use relative imports, they tend to break in obscure ways
How so? I thought Python 2 does relative imports by default, and only falls back to absolute imports if the relative import fails. So I would have assumed that a relative import would be on the safe side, whereas an absolute one could break in cases where there was a relative import of the same name available.
Anyway, I simply take your word for it, and changed everything to absolute paths. Don't want to do the same for the standalone distribution, though, as there people might choose a different plugin name.
> try to use [module imports instead of class imports]
Done.