Paths

Table of Contentst

Modernize the script and regex linter
AbandonedPublic
Actions

Authored by wotte on May 12 2014, 10:25 PM.

Details

Reviewers

joshuaspence
epriestley

Group Reviewers

Blessed Reviewers

Summary

The script and regex linter didn't appear to behave well with the more 'modern' approach to lint configuration. I've taken a stab at modernizing it while attempting to retain backwards compatibility.

noidea

Feedback is welcome, I'm sure I've buggered something.

Test Plan

I don't have an 'old style' configuration of the script and regex linter, so I couldn't test that. I did test the following 'new' style configuration in my .arclint:

{
  "linters": {
    "checkpatch": {
      "type": "script-and-regex",
      "script-and-regex.script": "sh -c 'scripts/checkpatch.pl --terse --emacs -f $0 || true'",
      "script-and-regex.regex": "/^.*:(?P<line>.*): (?P<severity>(WARNING|ERROR)):(?P<message>.*)$/m"
    }
  }
}

and that seemed to work just fine.

Diff Detail

Repository

rARC Arcanist

Branch

arcpatch-D9084

Lint

Lint Warnings

Severity	Location	Code	Message
Warning	src/lint/linter/ArcanistScriptAndRegexLinter.php:11	TXT3	Line Too Long

Unit

Tests Passed

Build Status

Buildable 433
Build 433: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

wotte updated this revision to Diff 21587.May 12 2014, 10:25 PM

wotte retitled this revision from to Modernize the script and regex linter.

wotte updated this object.

wotte edited the test plan for this revision. (Show Details)

wotte added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 12 2014, 10:25 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B411: Diff 21587.May 12 2014, 10:27 PM

This is generally pretty good. I've made a few inlines.

It could be done separately, but should probably be done first, but I think that this linter should extend from ArcanistExternalLinter rather than ArcanistLinter.

src/lint/linter/ArcanistScriptAndRegexLinter.php
10–11	I think that you still want this to be quoted, in case the path contains spaces.
227	I'd just hardcode this value. Its cleaner and we do it everywhere else anyway.
230	We should probably move most, if not all, of the documentation that exists at the top of this file into these help blocks. The idea is that the user shouldnt need to look at the source code to work out how to use this thing, and maintaining the docs is not always fun.
244	Again, we should just hardcode this.
247	And this.
291	I think that `getDeprecatedConfig` should take care of this. If it doesn't, well it should.
315	As above, `getDeprecatedConfig` should take care of this.
326	`PhutilTypeSpec` can get probably do a better job of this.

This revision now requires changes to proceed.May 12 2014, 10:48 PM

Thanks for the feedback. I'll throw up an updated diff hopefully later tonight.

src/lint/linter/ArcanistScriptAndRegexLinter.php
10–11	I'll test a little locally. I found that if I quoted the string, the `$0` captured the calling process arg0, and not the filename.
291	I added this because the name of the option changes as you move to .arclint from `linter.scriptandregex.script` to `script-and-regex.script`. I'm sure we can move that just into the documentation.
326	I'll take a look.

joshuaspence added inline comments.May 12 2014, 11:01 PM

src/lint/linter/ArcanistScriptAndRegexLinter.php
10–11	Possibly `$0` here is meant to be `$1`... I'm not 100% sure as I haven't used this linter before.
291	Yeah, I sort of understand your motivation... but I think that we have handled this already. `ArcanistLinter` will already raise a deprecation warning in `getDeprecatedConfiguration` if the old config is used, so I suspect that you will be raising a second warning here.

wotte added inline comments.May 13 2014, 12:30 AM

src/lint/linter/ArcanistScriptAndRegexLinter.php
10–11	Nah, `$0` is correct. Essentially the `sh` command looks like this: sh -c '/some/command -with -options $0' arg0 arg1 arg2 in this case, the linter passes the filename in the zeroth position, and sh interprets the `$0` to have the value of `arg0`. Putting double quotes in there causes the calling shell (e.g., whatever php invokes, probably bash or sh) to interpret the $0 instead. There's probably some magical incantation of quoting that will work as expected; then again shell scripting has never been my forte.

wotte added inline comments.May 13 2014, 12:46 AM

src/lint/linter/ArcanistScriptAndRegexLinter.php
230	I'll move some stuff here, but it might be better to move the lions share to a diviner document. I'll make a separate diff for that unless there's objection.

Addressed feedback.

Harbormaster completed remote builds in B413: Diff 21589.May 13 2014, 1:19 AM

Let me figure out extending from ExternalLinter.

I'm not sure that ExternalLinter is entirely apropos in this situation - it ultimately implies far too much structure in the final generated command that might not be appropriate for all script based linters.

In D9084#20, @wotte wrote:

I'm not sure that ExternalLinter is entirely apropos in this situation - it ultimately implies far too much structure in the final generated command that might not be appropriate for all script based linters.

What do you mean exactly? The parseLinterOutput method should be able to parse anything that you need to.

In D9084#22, @joshuaspence wrote:

In D9084#20, @wotte wrote:

I'm not sure that ExternalLinter is entirely apropos in this situation - it ultimately implies far too much structure in the final generated command that might not be appropriate for all script based linters.

What do you mean exactly? The parseLinterOutput method should be able to parse anything that you need to.

It's not the parsing of the output, it's the building of the command line. The ExternalLinter seems to presume that the command it executes has the form

/path/to/interpreter  /path/to/script

It also presumes that interpreter and script exist on the filesystem - that may not be the case, if it's relying on shell builtins. The script based linter probably needs to have a bit more flexibility; the ability to redirect output, provide additional command line options, possibly complex quoting behavior, etc.

Updated in light of D9100.

Harbormaster completed remote builds in B433: Diff 21613.May 13 2014, 2:27 PM

Is moving to ExternalLinter required to move this forward?

I'll take a look at this, one sec...

I don't think this needs to extend ExternalLinter to land. It would be slightly more correct to do so, but we get 95% of the value with these changes, and move some stuff like the config deprecation forward nicely, and I don't think it's digging us any deeper to not move it yet.

Caught a couple of minor technical inlines, though -- particularly, setLinterConfigurationValue() and validateConfiguredRegex() seem to duplicate regexp validation code?

src/lint/linter/ArcanistScriptAndRegexLinter.php
258	Can we just call validateConfiguredRegex()?
259–260	I think this can be slightly simpler as: PhutilTypeSpec::newFromString('regex')->check($this->regex);
314–316	This makes less sense than it used to now that we're doing an explicit check, consider removing?

This revision now requires changes to proceed.May 16 2014, 4:15 PM

Thanks for the feedback - I'll slot some time this afternoon to address.

In D9084#29, @epriestley wrote:

I don't think this needs to extend ExternalLinter to land. It would be slightly more correct to do so, but we get 95% of the value with these changes, and move some stuff like the config deprecation forward nicely, and I don't think it's digging us any deeper to not move it yet.

Yeah I tend to agree with you. This linter probably should be rewritten as ArcanistExternalLinter, but we can do that separately.

src/lint/linter/ArcanistScriptAndRegexLinter.php
9–11	I think just remove this. It's not really necessary for us to comment every class with "read the docs".
39–40	As above.
72	The spacing here is unnecessary.
237	This should probably be `'type' => 'regex'`, which means that the value will be guaranteed to be a valid regular expression. This would be better than performing the check on line 259.
285–336	I think that both of these functions (`validateConfiguredScript` and `validateConfiguredRegex`) aren't really needed. I would move the `getDeprecatedConfiguration` logic to the `willLintPaths` function.

D10704 achieves the same thing and is more up-to-date.

I've been slammed with other stuff, so I've not been able to follow up. I'm happy to abandon in favor of D10704.

Revision Contents
Changeset List

Path

Size

src/

lint/

linter/

ArcanistScriptAndRegexLinter.php

318 lines

Diff 21613

View Options

src/lint/linter/ArcanistScriptAndRegexLinter.php

Modernize the script and regex linterAbandonedPublicActions