On Sun, Apr 22, 2018 at 08:32:28PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <m...@ttaylorr.com> wrote:
> > This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> > option builds upon previous commits to show the column number of the
> > first match on a non-context line.
>
> Imperative mood (and dropping unnecessary "builds upon previous"):
>
>     Teach 'git-grep(1)' a new option '--column-number' which shows the
>     column number of the first match on a non-context line.

Thanks. I am not used to writing in this mood, but have amended my
patches locally to conform to your proposed layout and have reworded
each to be in the imperative mood.

> > For example:
> >
> >   $ ./git-grep -n --column-number foo | head -n3
> >   .clang-format:51:14:# myFunction(foo, bar, baz);
> >   .clang-format:64:7:# int foo();
> >   .clang-format:75:8:# void foo()
> >
> > Now that configuration variables such as grep.columnNumber and
> > color.grep.columnNumber have a visible effect, we document them in this
> > patch as well.
>
> As mentioned in my review of patch 2, document the configuration
> variables in the patch which introduces them.

Thanks again, I have moved this introduction to the relevant patch.

> > While we're at it, change color.grep.linenumber to color.grep.lineNumber
> > to match the casing of nearby variables.
> >
> > Signed-off-by: Taylor Blau <m...@ttaylorr.com>
> > ---
> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> > @@ -99,6 +99,28 @@ do
> > +       test_expect_success "grep -w $L" '
> > +               ...
> > +       '
> > +
> > +       test_expect_success "grep -w $L" '
> > +               ...
> > +       '
> > +
> >         test_expect_success "grep -w $L" '
>
> I realize that several existing tests in this script are already
> guilty of this sin, but please give each new test a distinct title
> reflective of what it is actually testing in order to make it easier
> to correlate failed test output with the actual test code.

:-). I have changed this locally to indicate which is which in the hopes
that it will provide more clarity should these tests fail at any point.


Thanks,
Taylor

Reply via email to