Julian Foad wrote on Wed, Sep 22, 2010 at 09:43:26 +0100:
> Hyrum Wright wrote:
> > On Tue, Sep 21, 2010 at 10:25 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > > hwright_at_apache.org wrote on Fri, Sep 17, 2010 at 20:04:53 -0000:
> > >> * subversion/include/svn_client.h
> > >> (svn_client_cat2): Rewrite docstring (see r997639).
> [...]
> > >> *
> > >> * A brief word on operative and peg revisions.
> > >> *
> > >> + * If the kind of the peg revision is #svn_opt_revision_unspecified, then it
> > >> + * defaults to #svn_opt_revision_head for URLs and #svn_opt_revision_working
> > >> + * for local paths.
> > >> + *
> > >> * For deeper insight, please see the
> > >> * <a href="http://svnbook.red-bean.com/nightly/en/svn.advanced.pegrevs.html">
> > >> * Peg and Operative Revisions</a> section of the Subversion Book.
> > >> @@ -4687,26 +4691,30 @@ svn_client_ls(apr_hash_t **dirents,
> > >> */
> > >>
> > >> /**
> > >> - * Output the content of file identified by @a path_or_url and @a
> > >> - * revision to the stream @a out. The actual node revision selected
> > >> - * is determined by the path as it exists in @a peg_revision. If @a
> > >> - * peg_revision->kind is #svn_opt_revision_unspecified, then it defaults
> > >> - * to #svn_opt_revision_head for URLs or #svn_opt_revision_working
> > >> - * for WC targets.
> > >> - *
> > >> - * If @a path_or_url is not a local path, then if @a revision is of
> > >> - * kind #svn_opt_revision_previous (or some other kind that requires
> > >> - * a local path), an error will be returned, because the desired
> > >> - * revision cannot be determined.
> > >> - *
> > >> - * Use the authentication baton cached in @a ctx to authenticate against the
> > >> - * repository.
> > >> + * Output the content of a file.
> > >> + *
> > >> + * @param[in] out The stream to which the content will be written.
> > >> + * @param[in] path_or_url The path or URL of the file.
> > >> + * @param[in] peg_revision The peg revision.
> > >> + * @param[in] revision The operative revision.
> > >> + * @param[in] ctx The standard client context, used for possible
> > >> + * authentication.
> > >> + * @param[in] pool Used for any temporary allocation.
> > >> *
> > >> - * Perform all allocations from @a pool.
> > >
> > > Sorry, but I like the old style better. I could read it and actually
> > > understand what the function does; whereas text like
> > >
> > > * Output the content of a file.
> >
> > This is *exactly* what the function does, without all the extra fluff.
The function's name tells me that.
> > Everything else is just modifying behavior, several pieces of which
> > are common to many of our APIs, and so should have that documentation
> > extracted and linked to. I'm just trying to save the effort of
> > reading five paragraphs of prose to find out what a single boolean
> > argument does.
> >
I appreciate the intent is to improve. However, I don't see how
+ * @param[in] peg_revision The peg revision.
helps me know what to pass for the third formal parameter.
> > The other problem is that the prose can make it hard to pick up on
> > inconsistent behavior, when something (such as depth defaults) isn't
> > the same between APIs.
> >
We could use @note for these situations.
> Although the concise style appears to contain just as much information
> when read in its entirety, I think we have indeed lost something
> semantically in this translation. I think the reader of this
> description is liable to think:
>
> "Output? To the screen, or what?"
> "A file? Any old file, or what?"
>
> The reader then has to read the list of parameters to find out. In this
> case the first param happens to say that a stream is used for output,
> but the reader isn't sure that there isn't going to be another param
> further down the list that says something like "The file to copy to,
> which can be supplied instead of or as well as the stream."
>
> Similarly for the fact that it outputs a specified revision of a
> (versioned) file; the lack of that piece of information makes the
> interpretation of "a file" vague. Simply having a parameter documented
> as "The revision" doesn't properly make the connection, although in a
> relatively simple case like this the reader can infer that it's the
> revision of the file to be output.
>
> The original sentence "Output ... file identified by @a path_or_url and
> @a revision ... to a stream" made both of those implications clear.
>
> So perhaps we should aim to say:
>
> * Output the content of a specified revision of a file
> * to a stream.
>
> The extra words here would not be fluff or "modifying behaviour", but
> rather a more accurate description of the primary behaviour. And this
> would give the word "the" something to attach to when we later refer to
> "The peg revision", "The stream", and so on.
>
> Daniel, does that address your concerns?
>
Yes, I think. Here's a stab:
/**
* Write the content of a specified revision of a versioned node
* to a given stream.
*
* @param[in] out The given stream, where the content will be written.
* @param[in] path_or_url The path or URL of the node.
* @param[in] peg_revision The revision to look up @a path_or_url at.
* @param[in] revision The revision whose content is to be output.
* @param[in] ctx The standard client context, used for possible
* authentication (via @a ctx->auth_baton).
*
* @see <revs and pegrevs>, <authn>, <pools>
*/
svn_error_t *
svn_client_cat2(svn_stream_t *out,
const char *path_or_url,
const svn_opt_revision_t *peg_revision,
const svn_opt_revision_t *revision,
svn_client_ctx_t *ctx,
apr_pool_t *scratch_pool);
(where we define "node" somewhere to mean "(repos_relpath, peg_revision) tuple".)
> - Julian
>
>
> > > *
> > > * @param[in] out The stream to which the content will be written.
> > > * @param[in] path_or_url The path or URL of the file.
> > > * @param[in] peg_revision The peg revision.
> > >
> > > tells me virtually nothing more than the signature does.
> >
> > That being said, improvements are welcome, or we can just revert what
> > I've already done.
> >
> > -Hyrum
>
>
>
>
Received on 2010-09-22 12:15:21 CEST