Bug 89086

Comment on attachment 113365 [details] [review]
Document dbus-test-tool

Review of attachment 113365 [details] [review]:
-----------------------------------------------------------------

Seems to be missing fun CMake changes to cmake/doc/CMakeLists.txt.

xmlto gives me the following warnings when I run it:
$ xmlto man dbus-test-tool.1.xml 
Warn: meta author : no refentry/info/author                        dbus-test-tool
Note: meta author : see http://docbook.sf.net/el/author            dbus-test-tool
Warn: meta author : no author data, so inserted a fixme            dbus-test-tool

::: doc/dbus-test-tool.1.xml.in
@@ +111,5 @@
> +      <title>black-hole mode</title>
> +      <variablelist remap="TP">
> +
> +        <varlistentry>
> +          <term><option>--name=</option><replaceable>NAME</replaceable></term>

Shouldn’t the <replaceable> be inside the <option>?

Same in various places below.

@@ +158,5 @@
> +
> +        <varlistentry>
> +          <term><option>--dest=</option><replaceable>NAME</replaceable></term>
> +          <listitem>
> +            <para>Send method calls to <replaceable>NAME</replaceable>.

Where NAME is presumably a well-known or unique name?

@@ +178,5 @@
> +          <listitem>
> +            <para>Send <replaceable>N</replaceable> method calls before
> +              waiting for any replies, then send one new call per reply
> +              received, keeping <replaceable>N</replaceable> method calls
> +              "in flight" at all times until the <option>--count</option>

‘until <replaceable>N</replaceable> is reached’?

@@ +216,5 @@
> +        <varlistentry>
> +          <term><option>--string</option></term>
> +          <listitem>
> +            <para>The payload of each message is a UTF-8 string. This is the
> +              default.</para>

What’s in the string? → Link to --payload and --stdin.

@@ +223,5 @@
> +
> +        <varlistentry>
> +          <term><option>--bytes</option></term>
> +          <listitem>
> +            <para>The payload of each message is a byte-array.</para>

What’s in the byte array? → Link to --payload and --stdin.

(In reply to Philip Withnall from comment #1)
> Seems to be missing fun CMake changes to cmake/doc/CMakeLists.txt.

Fair point, but dbus-test-tool itself is not currently built under cmake either.

It isn't a big deal if the CMake build lags behind the recommended Autotools build a little - it's primarily there for Windows, which is something of a second-class citizen in dbus anyway.

> $ xmlto man dbus-test-tool.1.xml 
> Warn: meta author : no refentry/info/author                       

I saw that warning, and have chosen to ignore it: I prefer the parts of D-Bus that I've written to be seen as "owned" by the D-Bus project, rather than individual developers having "territory", and I think the other maintainers have a similar policy. (I should add a Collabora copyright notice, though - copyright information is more important than authorship here.)

> > +          <term><option>--name=</option><replaceable>NAME</replaceable></term>
> 
> Shouldn’t the <replaceable> be inside the <option>?

I ... don't think so? --name= is the literal option text whereas NAME is a placeholder; this would be .BI (alternate bold with italic) if I was writing raw man-page text. But if you happen to know what is considered good Docbook style, I'll go along with it.

The examples in http://www.tldp.org/HOWTO/DocBook-Install/using.html suggest that either way works in practice.

> Where NAME is presumably a well-known or unique name?

Yeah, could benefit from clarification.

> > +              received, keeping <replaceable>N</replaceable> method calls
> > +              "in flight" at all times until the <option>--count</option>
> 
> ‘until <replaceable>N</replaceable> is reached’?

No, I think that would be confusing: the argument to --queue=N and the argument to --count=N are not the same N. A typical example would be something like --count=1000 --queue=100, which would send 100 immediately, then keep 100 at a time in-flight until it had sent the 1000th message in response to the 900th reply, after which the number in-flight would drop off gradually until the 1000th reply.

> What’s in the string? → Link to --payload and --stdin.

Fair point.

Created attachment 113398 [details] [review]
man page fixes, to be squashed

Created attachment 113399 [details] [review]
Add dbus-test-tool and its man page to the CMake build system

Created attachment 113400 [details] [review]
Document dbus-test-tool

---
Squashed version of Attachment #113365 [details] and Attachment #113398 [details] if you prefer that

Comment on attachment 113398 [details] [review]
man page fixes, to be squashed

Review of attachment 113398 [details] [review]:
-----------------------------------------------------------------

++

Comment on attachment 113399 [details] [review]
Add dbus-test-tool and its man page to the CMake build system

Review of attachment 113399 [details] [review]:
-----------------------------------------------------------------

++

(In reply to Simon McVittie from comment #2)
> (In reply to Philip Withnall from comment #1)
> > Seems to be missing fun CMake changes to cmake/doc/CMakeLists.txt.
> 
> Fair point, but dbus-test-tool itself is not currently built under cmake
> either.
> 
> It isn't a big deal if the CMake build lags behind the recommended Autotools
> build a little - it's primarily there for Windows, which is something of a
> second-class citizen in dbus anyway.

OK, I’ll ignore CMake in future reviews then.

> > > +          <term><option>--name=</option><replaceable>NAME</replaceable></term>
> > 
> > Shouldn’t the <replaceable> be inside the <option>?
> 
> I ... don't think so? --name= is the literal option text whereas NAME is a
> placeholder; this would be .BI (alternate bold with italic) if I was writing
> raw man-page text. But if you happen to know what is considered good Docbook
> style, I'll go along with it.
> 
> The examples in http://www.tldp.org/HOWTO/DocBook-Install/using.html suggest
> that either way works in practice.

Let’s leave it unchanged then.

> > > +              received, keeping <replaceable>N</replaceable> method calls
> > > +              "in flight" at all times until the <option>--count</option>
> > 
> > ‘until <replaceable>N</replaceable> is reached’?
> 
> No, I think that would be confusing: the argument to --queue=N and the
> argument to --count=N are not the same N. A typical example would be
> something like --count=1000 --queue=100, which would send 100 immediately,
> then keep 100 at a time in-flight until it had sent the 1000th message in
> response to the 900th reply, after which the number in-flight would drop off
> gradually until the 1000th reply.

Understood.

Fixed in git for 1.9.12