Re: [PATCH v2 4/6] tests: Add linter check for pipe placement style
- From
Matthew DeVore <matvore@google.com>
- Date
- Sep 19, 2018, 17:39 UTC
- Message-ID
- <CAMfpvhJ21ZtpjcJvOH7s=5r496z4Fz+tw94ymGD54xe=0RJ8Eg@mail.gmail.com>
- In-Reply-To
- <CAPig+cTO=BJ+-6+73fDmdWX7axCZvsyKMxE0E1bhGX-YUFrR-w@mail.gmail.com>
On Mon, Sep 17, 2018 at 5:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 28 quoted lines
>
> On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore <matvore@google.com> wrote:
> > tests: Add linter check for pipe placement style
>
> Until now, the various "lint" checks have been for genuine portability
> problems (except perhaps 'test-lint-duplicates'). This new lint check
> makes style violations worthy of failing "make test". Is the indeed
> the direction we want to go? (Genuine question. I can formulate
> arguments for either side.)
>
> > ---
> > diff --git a/t/Makefile b/t/Makefile
> > @@ -101,6 +101,16 @@ test-lint-filenames:
> > +test-lint-pipes:
> > + @# Do not use \ to join lines when the next line starts with a
> > + @# pipe. Instead, end the prior line with the pipe, and allow that to
> > + @# join the lines implicitly.
> > + @bad="$$(${PERL_PATH} -n0e 'm/(\n[^\n|]+\\\n[\t ]+\|[^\n]*)/ and \
> > + print qq{$$ARGV:$$1\n\n}' $(T))"; \
> > + test -z "$$bad" || { \
> > + printf >&2 "pipe at start of line in file(s):\n%s\n" "$$bad"; \
> > + exit 1; }
>
> If we're going in the direction of linting style violations, then
> maybe generalize this by calling it "test-lint-style" rather than
> "test-lint-pipes", and perhaps move the body of the test to a new
> script check-shell-style.pl (or something), much as portability
> violations are housed in check-non-portable-shell.pl.I agree with moving this code to a separate file. I also notice that there is logic in a file called t/chainlint.sed which normalizes scripts for the purpose of lint checks (e.g. it removes heredocs) and I ought to be re-using this logic in order to make the new lint check robust.
I think I'd like to shelve this particular commit and come back to it once I have the time and I'm more familiar with the code base. It is not critical to this patchset.