You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/09/21 21:25:08 UTC
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
hwright@apache.org wrote on Fri, Sep 17, 2010 at 20:04:53 -0000:
> Author: hwright
> Date: Fri Sep 17 20:04:53 2010
> New Revision: 998296
>
> URL: http://svn.apache.org/viewvc?rev=998296&view=rev
> Log:
> * subversion/include/svn_client.h
> (svn_client_cat2): Rewrite docstring (see r997639).
>
> Modified:
> subversion/trunk/subversion/include/svn_client.h
>
> Modified: subversion/trunk/subversion/include/svn_client.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=998296&r1=998295&r2=998296&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_client.h (original)
> +++ subversion/trunk/subversion/include/svn_client.h Fri Sep 17 20:04:53 2010
> @@ -322,6 +322,10 @@ svn_client_get_ssl_client_cert_pw_prompt
> *
> * 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.
*
* @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.
$0.02,
Daniel
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum Wright wrote on Wed, Sep 22, 2010 at 21:07:10 +0100:
> On Wed, Sep 22, 2010 at 8:40 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:08:41 +0100:
> >> On Wed, Sep 22, 2010 at 11:33 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> >> > [1] For the [out] parameter, can we have @param[out,optional] and
> >> > @param[out,mandatory] notations, or do we have to say "may be NULL"
> >> > in the prose?)
> >>
> >> @param[out] is part of the doxygen markup (not just some arbitrary
> >> notation). I don't know what it would do in the face of extra values
> >> (see http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam)
> >>
> >
> > Well, the [mandatory] and [optional] could be a nice extension to that
> > syntax.
>
> Agreed, though I'm not sure the doxygen parser would handle it.
>
I didn't mean we should invent doxygen syntax, I meant it would be nice
if doxygen supported that syntax (and once it does --- not before that
--- we can use it).
> >> > [2] How about introducing:
> >> >
> >> > struct svn_ra_node_t {
> >> > const char *repos_relpath;
> >> > svn_revnum_t peg;
> >> > };
> >> >
> >> > struct svn_client_node_t {
> >> > const char *path_or_URL;
> >> > svn_opt_revision_t *peg;
> >> > };
> >> >
> >> > (that will also help make the docstrings clearer)
> >>
> >> You'd probably want to the revision in there too, much like we do for
> >> svn_client_copy_source_t. Both the peg revision and the operative
> >> revision are used to specify a node (though in the absence of one, the
> >> default is generally the other, I think).
> >>
> >
> > Yeah, I can argue to have those structs both with/without the operative
> > revision in them.
> >
> > Either way, what I had in mind was using these structs in APIs instead
> > of having separate 'path' and 'peg_revision' arguments. This will
> > simplify docstrings (we can say "the node" instead of "the path as it
> > existed at the peg revision"), and it's logically correct too (represent
> > logical tuples as structs: the 'node', as one unit, is the target of the
> > operation).
>
> Quick thought: are there APIs which take an array of target
> paths/urls, but only a single peg/operative revision pair?
Examples:
* 'svn cp' (takes array of copy_source_t)
* syntax (2) of 'svn log' (takes a single peg)
* svn_ra_get_mergeinfo() (I'm not sure what pegs it uses)
> Would these need yet another struct?
>
No, they can take an array of strings instead of array of node_t's.
> -Hyrum
RE: svn commit: r998296 - /subversion/trunk/subversion/include/svn_client.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hyrum@hyrumwright.org [mailto:hyrum@hyrumwright.org] On Behalf Of
> Hyrum Wright
> Sent: woensdag 22 september 2010 22:07
> To: Daniel Shahaf
> Cc: Julian Foad; dev@subversion.apache.org
> Subject: Re: svn commit: r998296 -
> /subversion/trunk/subversion/include/svn_client.h
>
> On Wed, Sep 22, 2010 at 8:40 PM, Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
> > Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:08:41 +0100:
> >> On Wed, Sep 22, 2010 at 11:33 AM, Daniel Shahaf
> <d....@daniel.shahaf.name> wrote:
> >> > [1] For the [out] parameter, can we have @param[out,optional] and
> >> > @param[out,mandatory] notations, or do we have to say "may be
> NULL"
> >> > in the prose?)
> >>
> >> @param[out] is part of the doxygen markup (not just some arbitrary
> >> notation). I don't know what it would do in the face of extra
> values
> >> (see http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam)
> >>
> >
> > Well, the [mandatory] and [optional] could be a nice extension to
> that
> > syntax.
>
> Agreed, though I'm not sure the doxygen parser would handle it.
>
> >> > [2] How about introducing:
> >> >
> >> > struct svn_ra_node_t {
> >> > const char *repos_relpath;
> >> > svn_revnum_t peg;
> >> > };
The ra layer works in most cases session relative, where the session root is
not necessary the repository root (See issue #3242)
Bert
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum Wright wrote on Wed, Sep 22, 2010 at 21:07:10 +0100:
> On Wed, Sep 22, 2010 at 8:40 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:08:41 +0100:
> >> On Wed, Sep 22, 2010 at 11:33 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> >> > [1] For the [out] parameter, can we have @param[out,optional] and
> >> > @param[out,mandatory] notations, or do we have to say "may be NULL"
> >> > in the prose?)
> >>
> >> @param[out] is part of the doxygen markup (not just some arbitrary
> >> notation). I don't know what it would do in the face of extra values
> >> (see http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam)
> >>
> >
> > Well, the [mandatory] and [optional] could be a nice extension to that
> > syntax.
>
> Agreed, though I'm not sure the doxygen parser would handle it.
>
I didn't mean we should invent doxygen syntax, I meant it would be nice
if doxygen supported that syntax (and once it does --- not before that
--- we can use it).
> >> > [2] How about introducing:
> >> >
> >> > struct svn_ra_node_t {
> >> > const char *repos_relpath;
> >> > svn_revnum_t peg;
> >> > };
> >> >
> >> > struct svn_client_node_t {
> >> > const char *path_or_URL;
> >> > svn_opt_revision_t *peg;
> >> > };
> >> >
> >> > (that will also help make the docstrings clearer)
> >>
> >> You'd probably want to the revision in there too, much like we do for
> >> svn_client_copy_source_t. Both the peg revision and the operative
> >> revision are used to specify a node (though in the absence of one, the
> >> default is generally the other, I think).
> >>
> >
> > Yeah, I can argue to have those structs both with/without the operative
> > revision in them.
> >
> > Either way, what I had in mind was using these structs in APIs instead
> > of having separate 'path' and 'peg_revision' arguments. This will
> > simplify docstrings (we can say "the node" instead of "the path as it
> > existed at the peg revision"), and it's logically correct too (represent
> > logical tuples as structs: the 'node', as one unit, is the target of the
> > operation).
>
> Quick thought: are there APIs which take an array of target
> paths/urls, but only a single peg/operative revision pair?
Examples:
* 'svn cp' (takes array of copy_source_t)
* syntax (2) of 'svn log' (takes a single peg)
* svn_ra_get_mergeinfo() (I'm not sure what pegs it uses)
> Would these need yet another struct?
>
No, they can take an array of strings instead of array of node_t's.
> -Hyrum
Re: svn commit: r998296 - /subversion/trunk/subversion/include/svn_client.h
Posted by Hyrum Wright <hw...@apache.org>.
On Wed, Sep 22, 2010 at 8:40 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:08:41 +0100:
>> On Wed, Sep 22, 2010 at 11:33 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> > [1] For the [out] parameter, can we have @param[out,optional] and
>> > @param[out,mandatory] notations, or do we have to say "may be NULL"
>> > in the prose?)
>>
>> @param[out] is part of the doxygen markup (not just some arbitrary
>> notation). I don't know what it would do in the face of extra values
>> (see http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam)
>>
>
> Well, the [mandatory] and [optional] could be a nice extension to that
> syntax.
Agreed, though I'm not sure the doxygen parser would handle it.
>> > [2] How about introducing:
>> >
>> > struct svn_ra_node_t {
>> > const char *repos_relpath;
>> > svn_revnum_t peg;
>> > };
>> >
>> > struct svn_client_node_t {
>> > const char *path_or_URL;
>> > svn_opt_revision_t *peg;
>> > };
>> >
>> > (that will also help make the docstrings clearer)
>>
>> You'd probably want to the revision in there too, much like we do for
>> svn_client_copy_source_t. Both the peg revision and the operative
>> revision are used to specify a node (though in the absence of one, the
>> default is generally the other, I think).
>>
>
> Yeah, I can argue to have those structs both with/without the operative
> revision in them.
>
> Either way, what I had in mind was using these structs in APIs instead
> of having separate 'path' and 'peg_revision' arguments. This will
> simplify docstrings (we can say "the node" instead of "the path as it
> existed at the peg revision"), and it's logically correct too (represent
> logical tuples as structs: the 'node', as one unit, is the target of the
> operation).
Quick thought: are there APIs which take an array of target
paths/urls, but only a single peg/operative revision pair? Would
these need yet another struct?
-Hyrum
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:08:41 +0100:
> On Wed, Sep 22, 2010 at 11:33 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > [1] For the [out] parameter, can we have @param[out,optional] and
> > @param[out,mandatory] notations, or do we have to say "may be NULL"
> > in the prose?)
>
> @param[out] is part of the doxygen markup (not just some arbitrary
> notation). I don't know what it would do in the face of extra values
> (see http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam)
>
Well, the [mandatory] and [optional] could be a nice extension to that
syntax.
> >
> > [2] How about introducing:
> >
> > struct svn_ra_node_t {
> > const char *repos_relpath;
> > svn_revnum_t peg;
> > };
> >
> > struct svn_client_node_t {
> > const char *path_or_URL;
> > svn_opt_revision_t *peg;
> > };
> >
> > (that will also help make the docstrings clearer)
>
> You'd probably want to the revision in there too, much like we do for
> svn_client_copy_source_t. Both the peg revision and the operative
> revision are used to specify a node (though in the absence of one, the
> default is generally the other, I think).
>
Yeah, I can argue to have those structs both with/without the operative
revision in them.
Either way, what I had in mind was using these structs in APIs instead
of having separate 'path' and 'peg_revision' arguments. This will
simplify docstrings (we can say "the node" instead of "the path as it
existed at the peg revision"), and it's logically correct too (represent
logical tuples as structs: the 'node', as one unit, is the target of the
operation).
Thoughts?
> -Hyrum
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:08:41 +0100:
> On Wed, Sep 22, 2010 at 11:33 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > [1] For the [out] parameter, can we have @param[out,optional] and
> > @param[out,mandatory] notations, or do we have to say "may be NULL"
> > in the prose?)
>
> @param[out] is part of the doxygen markup (not just some arbitrary
> notation). I don't know what it would do in the face of extra values
> (see http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam)
>
Well, the [mandatory] and [optional] could be a nice extension to that
syntax.
> >
> > [2] How about introducing:
> >
> > struct svn_ra_node_t {
> > const char *repos_relpath;
> > svn_revnum_t peg;
> > };
> >
> > struct svn_client_node_t {
> > const char *path_or_URL;
> > svn_opt_revision_t *peg;
> > };
> >
> > (that will also help make the docstrings clearer)
>
> You'd probably want to the revision in there too, much like we do for
> svn_client_copy_source_t. Both the peg revision and the operative
> revision are used to specify a node (though in the absence of one, the
> default is generally the other, I think).
>
Yeah, I can argue to have those structs both with/without the operative
revision in them.
Either way, what I had in mind was using these structs in APIs instead
of having separate 'path' and 'peg_revision' arguments. This will
simplify docstrings (we can say "the node" instead of "the path as it
existed at the peg revision"), and it's logically correct too (represent
logical tuples as structs: the 'node', as one unit, is the target of the
operation).
Thoughts?
> -Hyrum
Re: svn commit: r998296 - /subversion/trunk/subversion/include/svn_client.h
Posted by Hyrum Wright <hw...@apache.org>.
On Wed, Sep 22, 2010 at 11:33 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> And another stab:
>
> /**
> * Checkout a working copy from a repository.
> *
> * @param[out] result_rev
> * If non-NULL, the value of the revision checked out form
> * the repository [1]. (Useful when @a revision is of
> * a kind other than #svn_opt_revision_number.)
> * @param[in] URL The URL to checkout.
> * @param[in] peg_revision The revision to look up URL at.
> * @param[in] revision The revision of (URL,peg) to checkout. [2]
> * @param[in] path Where to create the new working copy.
> * @param[in] depth
> * Controls how many levels of file hierarchy to populate
> * in the new working copy. If #svn_depth_unknown,
> * then behave as for #svn_depth_infinity, except in the case
> * of resuming a previous checkout of @a path (i.e., updating),
> * in which case use the depth of the existing working copy.
> * @param[in] ignore_externals
> * If @c TRUE, don't process externals definitions as part
> * of this operation.
> * @see SVN_PROP_EXTERNALS
> * @param[in] allow_unver_obstructions
> * If @c TRUE, then tolerate existing
> * unversioned items that obstruct incoming paths. Only
> * obstructions of the same type (file or dir) as the added
> * item are tolerated. The text of obstructing files is left
> * as-is, effectively treating it as a user modification after
> * the checkout. Working properties of obstructing items are
> * set equal to the base properties. <br>
> (huh? "working props"? "base props"? come again?)
This is copied directly from the previous docstring. My hope is that
the new format helps us highlight (and fix) deficiencies such as this
one.
> * If @c FALSE, then raise an error if there are any unversioned
> * obstructing items.
> * @param[in] ctx
> * The standard client context, used for authentication and
> * notification.
> *
> * @return A pointer to an #svn_error_t of the type (this list is not
> * exhaustive): <br>
> * #SVN_ERR_UNSUPPORTED_FEATURE if @a URL refers to a file rather
> * than a directory; <br>
> * #SVN_ERR_RA_ILLEGAL_URL if @a URL does not exist; <br>
> * #SVN_ERR_CLIENT_BAD_REVISION if @a revision is not one of
> * #svn_opt_revision_number, #svn_opt_revision_head, or
> * #svn_opt_revision_date. <br>
> * If no error occurred, return #SVN_NO_ERROR.
> *
> * @since New in 1.5.
> *
> * @see #svn_depth_t <br> #svn_client_ctx_t <br> @ref clnt_revisions for
> * a discussion of operative and peg revisions.
> */
> svn_error_t *
> svn_client_checkout3(svn_revnum_t *result_rev,
> const char *URL,
> const char *path,
> const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *revision,
> svn_depth_t depth,
> svn_boolean_t ignore_externals,
> svn_boolean_t allow_unver_obstructions,
> svn_client_ctx_t *ctx,
> apr_pool_t *scratch_pool);
>
> [1] For the [out] parameter, can we have @param[out,optional] and
> @param[out,mandatory] notations, or do we have to say "may be NULL"
> in the prose?)
@param[out] is part of the doxygen markup (not just some arbitrary
notation). I don't know what it would do in the face of extra values
(see http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam)
>
> [2] How about introducing:
>
> struct svn_ra_node_t {
> const char *repos_relpath;
> svn_revnum_t peg;
> };
>
> struct svn_client_node_t {
> const char *path_or_URL;
> svn_opt_revision_t *peg;
> };
>
> (that will also help make the docstrings clearer)
You'd probably want to the revision in there too, much like we do for
svn_client_copy_source_t. Both the peg revision and the operative
revision are used to specify a node (though in the absence of one, the
default is generally the other, I think).
-Hyrum
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
And another stab:
/**
* Checkout a working copy from a repository.
*
* @param[out] result_rev
* If non-NULL, the value of the revision checked out form
* the repository [1]. (Useful when @a revision is of
* a kind other than #svn_opt_revision_number.)
* @param[in] URL The URL to checkout.
* @param[in] peg_revision The revision to look up URL at.
* @param[in] revision The revision of (URL,peg) to checkout. [2]
* @param[in] path Where to create the new working copy.
* @param[in] depth
* Controls how many levels of file hierarchy to populate
* in the new working copy. If #svn_depth_unknown,
* then behave as for #svn_depth_infinity, except in the case
* of resuming a previous checkout of @a path (i.e., updating),
* in which case use the depth of the existing working copy.
* @param[in] ignore_externals
* If @c TRUE, don't process externals definitions as part
* of this operation.
* @see SVN_PROP_EXTERNALS
* @param[in] allow_unver_obstructions
* If @c TRUE, then tolerate existing
* unversioned items that obstruct incoming paths. Only
* obstructions of the same type (file or dir) as the added
* item are tolerated. The text of obstructing files is left
* as-is, effectively treating it as a user modification after
* the checkout. Working properties of obstructing items are
* set equal to the base properties. <br>
(huh? "working props"? "base props"? come again?)
* If @c FALSE, then raise an error if there are any unversioned
* obstructing items.
* @param[in] ctx
* The standard client context, used for authentication and
* notification.
*
* @return A pointer to an #svn_error_t of the type (this list is not
* exhaustive): <br>
* #SVN_ERR_UNSUPPORTED_FEATURE if @a URL refers to a file rather
* than a directory; <br>
* #SVN_ERR_RA_ILLEGAL_URL if @a URL does not exist; <br>
* #SVN_ERR_CLIENT_BAD_REVISION if @a revision is not one of
* #svn_opt_revision_number, #svn_opt_revision_head, or
* #svn_opt_revision_date. <br>
* If no error occurred, return #SVN_NO_ERROR.
*
* @since New in 1.5.
*
* @see #svn_depth_t <br> #svn_client_ctx_t <br> @ref clnt_revisions for
* a discussion of operative and peg revisions.
*/
svn_error_t *
svn_client_checkout3(svn_revnum_t *result_rev,
const char *URL,
const char *path,
const svn_opt_revision_t *peg_revision,
const svn_opt_revision_t *revision,
svn_depth_t depth,
svn_boolean_t ignore_externals,
svn_boolean_t allow_unver_obstructions,
svn_client_ctx_t *ctx,
apr_pool_t *scratch_pool);
[1] For the [out] parameter, can we have @param[out,optional] and
@param[out,mandatory] notations, or do we have to say "may be NULL"
in the prose?)
[2] How about introducing:
struct svn_ra_node_t {
const char *repos_relpath;
svn_revnum_t peg;
};
struct svn_client_node_t {
const char *path_or_URL;
svn_opt_revision_t *peg;
};
(that will also help make the docstrings clearer)
?
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:14:35 +0100:
> On Wed, Sep 22, 2010 at 11:14 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Julian Foad wrote on Wed, Sep 22, 2010 at 09:43:26 +0100:
> >> 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.
That's a sane goal, of course. I'm just trying to ensure we don't spill
the baby with the bathwater: the doc strings should still /document/
their respective functions; they should explain what a function does,
and not just what standard terms and objects it uses. :-)
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum Wright wrote on Wed, Sep 22, 2010 at 12:14:35 +0100:
> On Wed, Sep 22, 2010 at 11:14 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Julian Foad wrote on Wed, Sep 22, 2010 at 09:43:26 +0100:
> >> 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.
That's a sane goal, of course. I'm just trying to ensure we don't spill
the baby with the bathwater: the doc strings should still /document/
their respective functions; they should explain what a function does,
and not just what standard terms and objects it uses. :-)
Re: svn commit: r998296 - /subversion/trunk/subversion/include/svn_client.h
Posted by Hyrum Wright <hw...@apache.org>.
On Wed, Sep 22, 2010 at 11:14 AM, Daniel Shahaf <d....@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....@daniel.shahaf.name> wrote:
>> > > hwright@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
>>
>>
>>
>>
>
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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....@daniel.shahaf.name> wrote:
> > > hwright@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
>
>
>
>
Re: svn commit: r998296 -
/subversion/trunk/subversion/include/svn_client.h
Posted by Julian Foad <ju...@wandisco.com>.
Hyrum Wright wrote:
> On Tue, Sep 21, 2010 at 10:25 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > hwright@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.
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?
- Julian
> This is *exactly* what the function does, without all the extra fluff.
> 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.
>
> 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.
>
> > *
> > * @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
Re: svn commit: r998296 - /subversion/trunk/subversion/include/svn_client.h
Posted by Hyrum Wright <hw...@apache.org>.
On Tue, Sep 21, 2010 at 10:25 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> hwright@apache.org wrote on Fri, Sep 17, 2010 at 20:04:53 -0000:
>> Author: hwright
>> Date: Fri Sep 17 20:04:53 2010
>> New Revision: 998296
>>
>> URL: http://svn.apache.org/viewvc?rev=998296&view=rev
>> Log:
>> * subversion/include/svn_client.h
>> (svn_client_cat2): Rewrite docstring (see r997639).
>>
>> Modified:
>> subversion/trunk/subversion/include/svn_client.h
>>
>> Modified: subversion/trunk/subversion/include/svn_client.h
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=998296&r1=998295&r2=998296&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/include/svn_client.h (original)
>> +++ subversion/trunk/subversion/include/svn_client.h Fri Sep 17 20:04:53 2010
>> @@ -322,6 +322,10 @@ svn_client_get_ssl_client_cert_pw_prompt
>> *
>> * 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.
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.
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.
> *
> * @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