[MERGE] bzr help disambiguation and bzr help PLUGINNAME

[Robert Collins]

On Fri, 2007-04-20 at 05:06 -0400, Martin Pool wrote:

Thanks for the review.

> Martin Pool has voted +1 (conditional).
> Status is now: Conditionally approved
> Comment:
> +    """An object to manage help in multiple indices.
> +
> +    This maintains a list of places to search for help. It is currently
> +    separate to the HelpTopicRegistry because of its ordered nature, 
> but
> +    possibly we should instread structure it as a search within the 
> registry
> +    and add ordering and searching facilities to the registry. The 
> registry
> +    would probably need to be restructured to support that cleanly 
> which is
> +    why this has been implemented in parallel even though it does as a 
> result
> +    permit searching for help in indexs which are not discoverable via
> +    'help topics'.
> 
> sp 'instead', 'indexes'

oops, I missed that. Actually, indices is the correct spelling, as
reflected earlier in the same text :(. Thanks.

> Part of this paragraph seems like a todo comment, not an instruction to 
> a user of this class?

Its trying to describe the rational around as documentation on the
class. Its not necessarily something that should change.

> The initial description seems redundant with the class name, maybe use 
> the first sentence of your second para instead.
> 
> I think the key things this needs to say but doesn't is that: each index 
> has a unique prefix string, such as "commands", and contains help topics 
> which can be listed or searched.  Just for the benefit of people trying 
> to understand the api.

Will add.

> 
> -            if help in help_commands.keys():
> +            if cmd_name in help_commands.keys():
> 
> So it looks like that was quite broken before, as the variable 'help' 
> here is not a name but rather a stringio?  No objection to the change.

Yes, it was broken, fixed on the way through.

> +            result = "Plugin '%s' has no docstring.\n" % 
> self.module.__name__
> 
> Incidentally, it might be good to mention the plugin's __file__ when we 
> have a problem.

Hmmm. I dont think so. This is a description to the user that not help
is available. 'bzr plugins' will show the path to the plugin already,
and devs wondering why their help isn't showing can trivially use that.

> +        self.assertStartsWith(helptext, 'usage:bzr Demo')
> 
> I would expect a space after the colon.

So would I. Looks like I broke this my mistake.

> I haven't read the tests in detail but the general idea seems good.

Thanks. I'll cleanup and merge first thing Monday morning.

Cheers,
Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070421/0ac600e9/attachment.pgp