From: Emily Shaffer <emilyshaffer@google.com>
To: Heba Waly <heba.waly@gmail.com>
Cc: Heba Waly via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/1] config: add documentation to config.h
Date: Tue, 22 Oct 2019 13:42:59 -0700 [thread overview]
Message-ID: <20191022204259.GC9323@google.com> (raw)
In-Reply-To: <CACg5j26DuAUm9WR9-4awF7BeGCy81d5kMhhcsePyp3Kxh2DTGg@mail.gmail.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
next prev parent reply other threads:[~2019-10-22 20:43 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-18 0:06 [PATCH 0/1] config: add documentation to config.h Heba Waly via GitGitGadget
2019-10-18 0:06 ` [PATCH 1/1] " Heba Waly via GitGitGadget
2019-10-18 22:07 ` Jonathan Tan
2019-10-20 8:05 ` Heba Waly
2019-10-18 22:51 ` Emily Shaffer
2019-10-20 8:35 ` Heba Waly
2019-10-22 20:42 ` Emily Shaffer [this message]
2019-10-22 7:05 ` [PATCH v2 0/1] [Outreachy] config: move " Heba Waly via GitGitGadget
2019-10-22 7:05 ` [PATCH v2 1/1] " Heba Waly via GitGitGadget
2019-10-22 20:59 ` Emily Shaffer
2019-10-23 2:14 ` Junio C Hamano
2019-10-23 4:55 ` Heba Waly
2019-10-23 5:30 ` [PATCH v3 0/1] [Outreachy] " Heba Waly via GitGitGadget
2019-10-23 5:30 ` [PATCH v3 1/1] " Heba Waly via GitGitGadget
2019-10-23 21:38 ` Emily Shaffer
2019-10-24 2:14 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191022204259.GC9323@google.com \
--to=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=heba.waly@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).