Page MenuHomePhabricator

Modernize the script and regex linter
AbandonedPublic

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
SeverityLocationCodeMessage
Warningsrc/lint/linter/ArcanistScriptAndRegexLinter.php:11TXT3Line Too Long
Unit
Tests Passed
Build Status
Buildable 433
Build 433: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

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.
joshuaspence added a reviewer: joshuaspence.
joshuaspence added a subscriber: joshuaspence.

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.

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.

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.

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.

wotte edited edge metadata.

Addressed feedback.

Let me figure out extending from ExternalLinter.

wotte requested a review of this revision.May 13 2014, 1:33 AM

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#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.

wotte edited edge metadata.

Updated in light of D9100.

Is moving to ExternalLinter required to move this forward?

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

epriestley edited edge metadata.

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.

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.

joshuaspence edited edge metadata.

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.