On Sun, Oct 20, 2019 at 09:35:17PM +1300, Heba Waly wrote: > On Sat, Oct 19, 2019 at 11:52 AM Emily Shaffer <emilyshaf...@google.com> > wrote: > > > > On Fri, Oct 18, 2019 at 12:06:59AM +0000, Heba Waly via GitGitGadget wrote: > > > From: Heba Waly <heba.w...@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.w...@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
