Bazaar

Code review comment for lp:~richard-wilbur/bzr/1540293-manpage

1540293-manpage
Merge into bzr.dev

Revision history for this message

Vincent Ladeuil (vila) wrote on 2016-02-03:

> I agree that we need tests, but how do you do that in documentation?

In the end, it's text. So I'll start simple and build from there.

In this case, write a test that trigger build the man page and find a way to catch the offending escpae sequences.

> Do we
> have infrastructure for that already?

bzrlib/tests/test_generate_docs.py

I realize it's minimal ;)

> Otherwise it seems like a lot of text
> parsing and haggling with rules.

Then let's not do that ;)

regexp matching is brittle but that would be a start.

>
> Honestly, I haven't built the documentation, yet. So I don't know for sure
> that it is fixed

Right, I'm with you on that ;) That's why I want to see tests so we build confidence into our doc building process.

> but from reading the code:
> 1. The first line assigns an initial value to doc without any side effects
> (nothing else is changed).
> 2. The second line overwrites the value assigned to doc in the first line by
> calling bzrlib.help_topics.help_as_plain_text() which man-escapes the text
> returned by cmd.help() (which from my reading of class Command is just
> cmd.__doc__ as in the first line).
> 3. The output of this function is subsequently passed through man_escape()
> before being written to the output file.

Right, those steps can (should ?) be translated into tests with the simplest assertions about the produced text.

>
> In the associated bug (lp:1540293) I noted that I ran across a document
> describing conventions for GNU/Linux man-pages (man/info man-pages). I have
> only started reading it but already found that the title (header?) line '.TH '
> is supposed to have the title in all capitals (we bzr in lowercase). Also, it
> always expects a date of the most recent "nontrivial" change in the form YYYY-
> MM-DD. That sounds very much like our datestamp.

That also sounds like a simple test.

>
> I'll keep reading and see what I can learn (and report here and/or in the
> bug).
>
> I am certainly interested in targetting lp:bzr/2.7. How would I accomplish
> that?

Your work is captured in your branch, as long as you've started from a revision older than 6615, you can file MPs against lp:bzr or lp:bzr/2.7.

No need to merge trunk, you're working on an isolated part so this can ultimately land where appropriate.

> I agree that we need tests, but how do you do that in documentation?

In the end, it's text. So I'll start simple and build from there.

In this case, write a test that trigger build the man page and find a way to catch the offending escpae sequences.

>  Do we
> have infrastructure for that already?

bzrlib/tests/test_generate_docs.py

I realize it's minimal ;)

>  Otherwise it seems like a lot of text
> parsing and haggling with rules.

Then let's not do that ;)

regexp matching is brittle but that would be a start.

> 
> Honestly, I haven't built the documentation, yet.  So I don't know for sure
> that it is fixed

Right, I'm with you on that ;) That's why I want to see tests so we build confidence into our doc building process.

> but from reading the code:
> 1.  The first line assigns an initial value to doc without any side effects
> (nothing else is changed).
> 2.  The second line overwrites the value assigned to doc in the first line by
> calling bzrlib.help_topics.help_as_plain_text() which man-escapes the text
> returned by cmd.help() (which from my reading of class Command is just
> cmd.__doc__ as in the first line).
> 3.  The output of this function is subsequently passed through man_escape()
> before being written to the output file.

Right, those steps can (should ?) be translated into tests with the simplest assertions about the produced text.

> 
> In the associated bug (lp:1540293) I noted that I ran across a document
> describing conventions for GNU/Linux man-pages (man/info man-pages).  I have
> only started reading it but already found that the title (header?) line '.TH '
> is supposed to have the title in all capitals (we bzr in lowercase).  Also, it
> always expects a date of the most recent "nontrivial" change in the form YYYY-
> MM-DD.  That sounds very much like our datestamp.

That also sounds like a simple test.

> 
> I'll keep reading and see what I can learn (and report here and/or in the
> bug).
> 
> I am certainly interested in targetting lp:bzr/2.7.  How would I accomplish
> that?

Your work is captured in your branch, as long as you've started from a revision older than 6615, you can file MPs against lp:bzr or lp:bzr/2.7.

No need to merge trunk, you're working on an isolated part so this can ultimately land where appropriate.

« Back to merge proposal