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

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

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

Ok, good enough for now.
<snip/>

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

Fine for me, thanks for sharing your thoughts on the subject.

> I guess I'll keep the plugin around for about a year or so,

Sounds reasonable.

> Done, and it does look better. Thanks!

Excellent.

>
> > but 'Approved' with the tweaks mentioned above.
>
> Why the plural?

Because my English is not precise enough I suspect :)

> I'd consider the assertion stuff a single tweak,

Yup, that's what I was referring to.

 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.

Correct.

review: Approve

« Back to merge proposal