On Tue, 2010-05-04 at 23:23 +0200, Hyrum K. Wright wrote:
> On Tue, May 4, 2010 at 12:36 PM, Julian Foad <julian.foad_at_wandisco.com>wrote:
> > Jumping in at this point, I can't grok this doc string. Can you write
> > it as a statement of what this function (a function of this type) is
> > required to do or promises to do or is expected to do? Try not to
> > assume the meanings of the parameters are totally obvious, and avoid
> > saying "return" when describing a parameter. ("const char *" is not
> > normally associated with "return" - are those parameters outputs?)
>
> A agree that the docstring is a bit confusing, but I'll make a couple of
> comments about it. This docstring actually is *more* explanatory than
> docstrings for similar callbacks elsewhere in svn_client.h. The problem is
> more rampant than just this one (though that does not excuse sloppiness in
> this case.)
>
> Asking what this function promises or is expected to do is itself a bit
> confusing. For instance, what does the blame receiver promise? Most of the
> callbacks in the client are informational. As this one is similar to the
> others, who are similarly documented, I assumed that aspect was apparent.
Hi Hyrum.
Sorry that my email sounded harsh - my long list of questions made it
look like I had a lot of problems with it, when in fact it probably just
needed a little clarification.
I don't expect any doc string to spell out the answers in full to all
the questions I asked. It should just say enough to enable the reader
to be confident of the answers. I had lots of questions because I
didn't know what's relevant, largely because I was confused due to
having an expectation that this callback played a part in *controlling*
the behaviour of the patching process. From the other emails I
understand that's not the case, it's a purely informational callback.
I think it's always fair to ask "What does this callback promise?" (as
well as "What should it expect?"). Maybe it promises not to modify the
target file. Maybe it doesn't promise anything. I don't know whether
the blame receiver promises anything. It's fine for it not to, and it's
fine not to mention any promises if it doesn't seem relevant.
> > For example, is it guaranteed to be called before or after the target is
> > patched? Is function able to examine the patch? Is it allowed to
> > modify the target file before patching? Should it or can it attempt to
> > apply the patch itself, or adjust the patch, or reject the patch, or
> > cause this patch to get applied to a different target? How can this
> > function, or how can the caller of svn_client_patch(), know whether
> > patch application has been successful?
[...]
> > Wasn't the idea that the caller could choose whether the patched result
> > should be written straight to the target file or to somewhere else? I'm
> > not sure but I thought specifying @a patched_tempfiles != NULL caused
> > that to happen. Is that right and if so does the callback support that
> > feature?
>
> I think Stefan addressed this already.
Yup.
[...]
> Thanks for the review. I committed an update to the docstrings in r941049.
> Unfortunately, I'm also late to the party when it comes to the internals of
> how the patch code works, so I'd suggest that further improvements should be
> made by those who have the most familiarity with the code.
Thanks, r941049 is better.
- Julian
Received on 2010-05-05 11:14:44 CEST