On Wed, Jun 19, 2019 at 7:36 PM Emily Shaffer <emilyshaffer@xxxxxxxxxx> wrote:
> On Wed, Jun 19, 2019 at 04:13:35AM -0400, Eric Sunshine wrote:
> > Maybe I got confused because the tiny cmd_walken() snippets followed
> > one another so closely (or because I got interrupted several times
> > during the review), but one way to avoid that would be to present a
> > single _complete_ snippet from the start, followed by a bit of
> > explanation. [...]
>
> Hmm. I can say that I personally would find that much more difficult to
> follow interactively, and I'd be tempted to copy-and-paste and skim
> through the wall of text if I was presented with such a snippet.
> However, I could also imagine the reverse - someone becoming tired of
> having their hand held through a fairly straightforward implementation,
> when they're perfectly capable of reading a long description and would
> just like to get on with it.
>
> (Maybe we can split the difference and present a complete patch or new
> function, followed by a breakdown? That would end up even more verbose
> than the current approach, though.)
It might not be that important and may not need fixing considering
that I read it correctly the second time, and don't know how I managed
to get confused on the first read.
> > As this is just a toy example, I don't care too strongly about the
> > unnecessary second sentence. On the other hand, the tutorial is trying
> > to teach people how to contribute to this project, and on this
> > project, that sort of pointless comment is likely to be called out in
> > review. In fact, given that view, the entire comment block is
> > unnecessary (it doesn't add any value for anyone reviewing or reading
> > the code), so it might make more sense to drop the comment from the
> > code entirely, and just do a better job explaining in prose above the
> > snippet why you are calling that function. For instance:
> >
> > ... Let's start the helper with the call to `prepare_revision_walk()`,
> > which does the final setup of the `rev_info` structure before it can
> > be used.
> >
> > The above observation may be more widely applicable than to just this
> > one instance. Don't use in-code comments for what should be explained
> > in prose if the in-code comment adds no value to the code itself (to
> > wit, if a reviewer would say "don't repeat in a comment what the code
> > already says clearly" or "don't use a comment to state the obvious").
>
> I'm of two minds about this. On the one hand, I'm somewhat in favor of
> leaving contextual, informational comments in the sample code, so the
> sample code can teach on its own without the tutorial (specifically, I
> mean the patchset that was sent alongside this one as RFC). On the other
> hand, you're right that adding these informational comments doesn't
> model best practices for real commits.
>
> I don't have a strong opposition to removing those comments from the
> in-place samples in the tutorial itself. But I do think it's useful to
> include them in the sample patchset, which is intended as an additional
> learning tool, rather than as a pristine code example - especially if we
> make it clear in the commit messages there.
Indeed, having the comments in the sample patch-set makes sense for
people who learn better that way (by seeing a complete piece of code).
> > > > Or make the output more useful by having it be machine-parseable (and
> > > > not localized):
> > > >
> > > > printf("commits %d\nblobs %d\ntags %d\ntrees %d\n",
> > > > commit_count, blob_count, tag_cont, tree_count);
> > >
> > > I'm not sure whether I agree, since it's a useless toy command only for human
> > > parsing.
> >
> > True, it's not a big deal, and I don't insist upon it. But, if you
> > mention in prose that this output is easily machine-parseable, then
> > perhaps that nudges the reader a bit in the direction of thinking
> > about porcelain vs. plumbing, which is something a contributor to this
> > project eventually has to be concerned with (the sooner, the better).
>
> Oh, that's a very good point. I'll frame it that way - that's a handy
> place to slip in some bonus context about Git. Thanks.
>
> NOTE: We aren't localizing the printf here because we have purposefully
> formatted it in a machine-parseable way. Commands in Git are divided into
> "plumbing" and "porcelain"; the "plumbing" commands are machine-parseable and
> intended for use in scripts, while the "porcelain" commands are intended for
> human interaction. Output intended for script usage doesn't need to be
> localized; output intended for humans does.
I'd go with stronger language than "doesn't need to be localized" and
say instead that plumbing output "must not be localized" since scripts
depend upon stable output (and stable API).
