Re: property code cleanup

On Tue, Nov 06, 2001 at 11:27:43AM -0600, Karl Fogel wrote:
> Kevin Pilch-Bisson <kevin@pilch-bisson.net> writes:
> > I wasn't aware of that, and indeed was comtemplating using either a table
> > or a hash.
>
> Gotcha. Hmmm, if you can think of any appropriate place(s) where this
> important fact -- about prop values permitting binary -- should be
> stated, go ahead and add it there.
>
I can't think of anything. However, I do have a question. How are we going
to deal with printing binary props? Currently we just printf the property
value, which won't work for binary properties.
>
> > Thinking of a doc-string along the following lines:
> >
> > /* Returns an apr_hash_t where the key is a char * representing the path
> > from the pwd to the node on which property PROPNAME is set, and the key
> > is the value of PROPNAME on that node. If TARGET is a file or RECURSE is
> > false, there will be only a single element in *PROPS, with key TARGET->data.
> > */
> >
> > Comments?
>
> Sure. This is going to be fairly picky, so please keep in mind the
> wise words Jim Blandy said to me many years ago (before totally taking
> apart some Elisp code I'd written): "A sincere and thorough critique
> is the highest form of flattery."

Don't worry about my feelings. If I hadn't wanted criticism for the doc string,
I wouldn't have asked for comments. :) Thanks for being honest about it.

>
> First, put the doc string in the direct, active voice (see example
> below) :-). But more importantly, it needs to be much, much more
> precise. For example, `pwd' is a meaningless concept in this
> function, as in most of Subversion. What you really mean is that the
> keys are paths relative to TARGET (I think). And what types are the
> property values? It doesn't say. What does it do when the property
> is absent on a given entity? It doesn't say.

Good points, all.

>
> Here's a possible doc string; I think it's factually correct, but it
> may still have errors. Feel free to use it or just treat it as a
> model, as you find appropriate:

Excellent, and almost correct.

>
> /* Set *PROPS to a hash table whose keys are `char *' paths
> * indicating the relative path from TARGET to a node on which

Actually, the keys represent the relative path from where the command is
called. The path starts as TARGET, and gets new filename or dirnames added
to it as it recurses. Should I state:

"whose keys are `char *' paths indicating the relative path, including TARGET,
to a node on which property PROPNAME is set" ?

Alternatively, the function could be modified to perform as your docstring
suggests.

> * property PROPNAME is set, and whose values are `svn_string_t *'
> * representing the property value for PROPNAME at that path.
> * Allocate *PROPS, its keys, and its values in POOL.
> *
> * Don't store any path, not even TARGET, if it does not have a
> * property named PROPNAME.
> *
> * If TARGET is a file or RECURSE is false, *PROPS will have at most
> * one element.
> *
> * If error, don't touch *PROPS, otherwise *PROPS is a hash table
> * even if empty.
> */
>
> Note how certain specifities are deliberately *not* included:
>
> It doesn't say "Set *PROPS to an `apr_hash_t *'", because the
> prototype of the function determines that itself. A doc string should
> write out a C type only when the function prototype won't determine
> that type. When the prototype does determine, it would be redundant
> (and distracting) to use anything other than regular prose at that
> point in the documentation.
>
> It also avoids referring unnecessarily to inner members of arguments
> or return values (e.g., "TARGET->data"). Again, such precision is
> attention-getting, and therefore should only be used for things that
> actually deserve the extra attention, or which cannot be deduced any
> other way.
>
> Hope this helps.

Definitely.

>
> > I thought of having a hash of hashes (or table of hashes), similar
> > to proplist, but thought this would be redundant, since we are only
> > getting a single property.
>
> Agree, yeah. Go with the shallowest data type that does the job.
>

My only reason for thinking of the other approach is that given the amount
of code we have which creates a hash from propname->propval, I thought it might
be counter intuitive to have one function which returns a hash mapping
nodename->propval. All in all, though, it is the simplest approach, and with
a good docstring it shouldn't be confusing :)

-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson                    http://www.pilch-bisson.net
     "Historically speaking, the presences of wheels in Unix
     has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~