Weekly update: 0.12

Sun Oct 15 20:14:09 BST 2006

On Thu, 2006-10-12 at 11:03 +1000, John Arbash Meinel wrote:
> Goffredo Baroncelli wrote:
> > Hi all,
> > 
> > On Monday 09 October 2006 02:31, John Arbash Meinel wrote:
> >> help topics - There has been a couple patches which seem to have
> >>    stalled. Goffredo submitted some updates that did not seem to get
> >>    reviewed. I'm hoping he can re-submit them to the mailing list, and
> >>    we can get this landed
> >  
> > enclosed you can find the bundle updated to the latest bzr.dev.
> > Please review it.
> > Thank you
> > 
> > Goffredo
> 
> I cleaned up a couple of small PEP8 things (spacing, doc strings, etc).
> But otherwise +1 from me.
> 
> I'm attaching the new diff, in case someone wants to review it. The
> branch is available from:
> 
> http://bzr.arbash-meinel.com/branches/bzr/0.12-working/help-topics
> 
> John
> =:->
> 

The semantics and syntax are both somewhat confused.  I think this is a
good direction for the design of bzr, but I believe some clarification
is needed.

+0 as it stands.

With some modifications I would be willing to vote a hearty +1.

> plain text document attachment (help-topics.diff)
> === added file 'bzrlib/help_topics.py'
> +_HELP_TOPICS={}
> +
> +
> +def add_topic(name, obj, comment):
> +    """add a new topic, obj can be a function or a text; comment is a text"""
> +    _HELP_TOPICS[name]=(obj, comment)
From the way this is used below, I think it would help implementers to
mention the purpose of the parameters.  Maybe something like ...
"""Add documentation for a new topic.

:param name:  Topic of documentation entry.
:param obj:  Function or string object providing detailed documentation.
Function interface is obj(name, outfile).
:param comment:  String providing documentation (single-line?) summary.

"""

Might even consider renaming the parameters to (topic, detail, summary)?
In which case, doc string
"""Add documentation for a new topic.

:param topic:  Name of documentation entry.
:param detail:  Function or string object providing detailed
documentation for topic.  Function interface is detail(topic, outfile).
:param summary:  String providing single-line documentation for topic.

"""

[...]
> +def get_topics_list( ):
> +    """return a dict like {topic_name:topi_comment}"""
> +    return _HELP_TOPICS
Why is this called 'get_topics_list()' when the doc string and the
return statement agree that it returns a dict?  _HELP_TOPICS is
initialized as an empty dict at the top of this code.  Maybe call it
'get_topics_dict()' or 'get_help_topics()'?

The doc string might better read
 """return a dict like {topic_name:(topic_object, topic_comment)}"""
In light of comments above, possibly
 """return a dict like {topic:(detail, summary)}"""

Alternate function definition
def get_topics_list():
    """return a list of all documented topics"""
    return _HELP_TOPICS.keys()

> +def _help_on_topics(name, outfile):
> +    """Write out the help for topics to outfile"""
> +
> +    topics = get_topics_list()
> +    lmax = max(len(topic) for topic in topics)
> +        
> +    for topic in topics:
> +        obj, comment = topics[topic]
> +        spaces = " " * (lmax-len(topic))
> +        outfile.write("%s%s %s\n" % (topic, spaces, comment))

In view of the discussion above, I propose a new version
def _help_on_topics(name, outfile):
    """Write the summary help for all documented topics to outfile."""

    topics_dict = get_topics_dict()
    topics = get_topics_list()
    lmax = max(len(topic) for topic in topics)

    for topic in topics:
        detail, summary = topics_dict[topic]
        spaces = " " * (lmax-len(topic))
        outfile.write("%s%s %s\n" % (topic, spaces, summary))

> +def _help_on_revisionspec(name, outfile):
> +    """Write out the help for revison spec information"""
                                 ^^^^^^^- revision

> === modified file 'bzrlib/revisionspec.py'
> --- bzrlib/revisionspec.py	2006-09-03 15:58:15 +0000
> +++ bzrlib/revisionspec.py	2006-10-11 23:20:23 +0000
> @@ -293,6 +310,14 @@
>  class RevisionSpec_revid(RevisionSpec):
> +    """Selects a revision using the revision id.
> [...]
> +    examples:
> +      revid:aaaa at bbbb-123456789 -> Select revision 'aaa at bbb-123456789'
> +    """    
Why does it select revision 'aaa at bbb' when you specify 'aaaa at bbbb'?

> @@ -306,6 +331,14 @@
>  class RevisionSpec_last(RevisionSpec):
> +    """Selects the nth revision from the end.
> +
> +    Supply a positive number to get the nth revision from the end.
> +    This is the same as suppling negative numbers to the 'revno:' spec.
                           ^^^^^^^^- supplying

> @@ -334,6 +367,21 @@
>  class RevisionSpec_before(RevisionSpec):
> +    """Selects the parent of the revision specified.
> [...]
> +    examples:
> +      before:1913    -> Return the parent of revno 1913 (revno 1912)
> +      before:revid:aaaa at bbbb-1234567890  -> return the parent of revision
> +                                            aaa at bbb-1234567890
Why does it return the parent of revision 'aaa at bbb' when you specify
'aaaa at bbbb'?

> @@ -405,16 +469,15 @@
>      def _match_on(self, branch, revs):
> [...]
> -          So the proper way of saying 'give me all entries for today' is:
> -              -r date:yesterday..date:today
>          """
> +        #  XXX: This doesn't actually work
> +        #  So the proper way of saying 'give me all entries for today' is:
> +        #      -r date:yesterday..date:today
So what is the proper way of saying 'give me all entries for today'?

> @@ -467,6 +530,22 @@
>  class RevisionSpec_ancestor(RevisionSpec):
> +    """Selects a common ancestor with a second branch.
> [..]
> +    examples:
> +      ancestor:/path/to/branch
> +      bzr diff -r ancestor:../../mainline/branch
> +    """
The second line in the example looks like a command line, the first
doesn't.  Should the second line have a '%' or '$' or '>' prompt
prepended to it to clarify the difference?

> === modified file 'bzrlib/tests/blackbox/test_help.py'
> --- bzrlib/tests/blackbox/test_help.py	2006-07-30 07:02:22 +0000
> +++ bzrlib/tests/blackbox/test_help.py	2006-10-11 22:01:55 +0000
> @@ -32,6 +32,18 @@
> +    def test_help_topics(self):
> +        """Smoketest for 'bzr help topics'"""
> +        out, err = self.run_bzr('help', 'topics')
> +        self.assertContainsRe(out, 'basic')
> +        self.assertContainsRe(out, 'topics')
What about 'revisionspec'?

> +    def test_help_revisionspec(self):
> +        """Smoke test for 'bzr help revisionspec'"""
> +        out, err = self.run_bzr('help', 'revisionspec')
> +        self.assertContainsRe(out, 'revno:')
> +        self.assertContainsRe(out, 'date:')
> +
What about 'revid:', 'last:', 'before:', 'ancestor:', and 'branch:'?
Are classes that provide these missing from
bzrlib.revisionspec.SPEC_TYPES?

Look forward to your feedback.

Sincerely,

Richard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061015/ef77339e/attachment.pgp 