On Wed, Sep 22, 2010 at 11:14 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> 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.
As it should. :) (But really, for a non-*nix programmer, 'cat'
doesn't exactly equate to 'output contents'.)
>> > 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.
We could. In my first pass, I just collected them all in the @see
paragraph, but we could more closely locate the references to the
actual usage (though for things like peg and operative revisions, we'd
be duplicating again).
>> 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.
If the extra prose here helps, that's great. We don't have to be
minimalist, just consistent. I'd hope we can use the same text for
every API which takes a peg revision or revision. My goal, at least,
is for people to be able to look at an API and think "ah, I know what
those are, because I've used them in these other places over hear" and
have that be valid, because the docs (and underlying behavior) is
sanely consistent.
> * @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 13:15:30 CEST