Re: [PATCH 1/1] config: add documentation to config.h

[Emily Shaffer <emilyshaffer@google.com>]

On Sun, Oct 20, 2019 at 09:35:17PM +1300, Heba Waly wrote:
> On Sat, Oct 19, 2019 at 11:52 AM Emily Shaffer <emilyshaffer@google.com> wrote:
> >
> > On Fri, Oct 18, 2019 at 12:06:59AM +0000, Heba Waly via GitGitGadget wrote:
> > > From: Heba Waly <heba.waly@gmail.com>
> >
> > Hi Heba,
> >
> > Thanks for the patch!
> >
> > I'd like to highlight to the community that this is an Outreachy
> > applicant and microproject. Heba, when you send the next version, I
> > think you can add [Outreachy] manually to the PR subject line - that
> > should draw the attention of those in the community who are invested in
> > helping Outreachy applicants.
> Good idea! I wanted to add it to the email subject but as I decided to
> use gitgadget
> I had no control over the subject.

Hm, it looks like you already figured out how to add it to the title of
the PR. :)

> > >
> > > This commit is copying and summarizing the documentation from
> > > documentation/technical/api-config.txt to comments in config.h
> >
> > I think in the GitGitGadget PR you've got some great comments from Dscho
> > about how to format your commit message; please take a look at those and
> > feel free to reach out to me if you're still not sure what's missing or
> > not.
> Will do.
> > > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> >
> > One thing I miss in this change is the removal of the contents of
> > Documentation/technical/api-config.txt (or maybe the removal of the file
> > itself). I'd prefer to see at least for api-config.txt to say something
> > like "Please refer to comments in 'config.h'"; or, more drastically, for
> > api-config.txt to be removed entirely.
> >
> > Having both pieces of documentation standing independently means that
> > someone who's trying to add new information about the config API won't
> > know where to add it; eventually they'll add something to config.h but
> > not api-config.txt, or vice versa, and the two documents will go out of
> > sync. So we want to move the documentation, rather than copy it.
> That makes sense, thanks for the explanation.
> I wasn't sure if it should be removed or not so I decided to leave it
> until I'm asked otherwise.
> So I assume api-config.html will be removed too?

That shouldn't be tracked - this is generated from api-config.txt as
part of the build. So don't worry about this part.

> > > +
> > > +/**
> > > + * Value Parsing Helpers
> > > + * ---------------------
> >
> > It may not make sense to have the header here in the middle of the doc.
> >
> > I wonder whether we need the headers at all anymore; or, whether it
> > makes more sense to put this header in the long comment at the top with
> > just the list of function names (so someone knows where to look), and
> > leave the per-function explanations inline with the function they
> > describe?
> I see your point Emily, but in the CodingGuidelines file it was
> advised to refer to strbuf.h
> as a model for documentation, I noticed that strbuf.h used headers
> this way so I decided
> to replicate that.

Ok! Sure.

> > I made a couple of smallish comments about general formatting, but I'm
> > also interested to know whether you were able to move the entire
> > contents of api-config.txt across to here. Was there anything that you
> > couldn't find a place for?
> Yes, everything is moved.
> > Thanks a lot for this change, and congrats on getting your first review
> > out! Welcome! :)
> >
> >  - Emily
> >
> Thanks a lot Emily for the detailed and helpful feedback!
> 
> Heba