[MERGE] bzr help disambiguation and bzr help PLUGINNAME

[Robert Collins]

On Fri, 2007-04-20 at 17:29 +0100, James Westby wrote:
> On (20/04/07 15:49), Robert Collins wrote:
> > This adds support for bzr help to identify when topics from areas like
> > 'registered topics' and 'plugins' and 'commands' overlap, and report
> > this to the user.
> 
> I like the general idea. It allows much more information to be provided,
> even if there isn't going to be that much yet.
> 
> > +def _get_cmd_object(cmd_name, plugins_override=True):
> > +    """Worker for get_cmd_object which raises KeyError rather than BzrCommandError."""
> 
> Long line?

Yes. as PEP8 says pragmatism first; The first line should be a complete
sentence, and can't be split because summaries no longer show right. 

> > +        if doc is None:
> > +            raise NotImplementedError("sorry, no detailed help yet for %r" % self.name())
> 
> Another long line.

This one was just moved code..

> I hit this the other day with a plugin. It means you get a traceback if
> there is no help provided, which is quite ugly, and perhaps a little
> over the top. On the other hand it may provide more incentive to
> actually provide some help.

That I actually think should be deleted. I think we should do what I do
for plugins with no docstring: explain that there is no help and hint at
how it can be added.

> > +        see_also = self.get_see_also(additional_see_also)
> > +        if see_also:
> > +            result += '\nSee also: '
> > +            result += ', '.join(see_also)
> > +            result += '\n'
> > +        return result
> 
> One thing this does is make all references appear to have the same relevance,
> one because they are sorted, the other because you removed direct
> references to use this system.
> 
> While it is probably a small thing I think in some cases it makes
> things slightly non-obvious. For instance the checkouts help topic
> attempts to explain what a checkout is and how it differs from a branch.
> With this patch 'bzr help checkout' shows
...
> There could be a couple of possible solutions. We could add the prefixes
> to the 'See also:' line, so at least you know whether it is a topic or
> command, or whatever. The other could be to add back in some manual
> reference text for the cases that could be highlighted.

I agree. I didn't remove all manual cross references for this reason. Do
you think we should add this particular one back? I'm not inclined to
add typing of references at this point.

> > +
> > +class NoHelpTopic(BzrError):
> > +
> > +    _fmt = ("No help could be found for '%(topic)s'. "
> > +        "Please use 'bzr help topics' to obtain a list of topics.")
> > +
> 
> I feel this should perhaps mention commands. If the user is aiming to
> get the help for a command and mistypes it might be a little cryptic.
> However having the error might be enough to prompt them to check.

I could go either way. But I think pathologically we end up listing
everything there. The basic issue is that for a simple term like 'foo'
we cannot tell what index the user was hoping to look in. If the user
says 'commands/foo' then we do know - and thats actually a good future
improvement: If the user provides a qualified search, give a qualified
error.

> Overall I think it is a good change, so I'm +1 on the idea, if that
> counts for anything.

It does - thanks for the review.

-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/5aff31c0/attachment.pgp