Page MenuHomePhabricator

Modernize Script-and-Regex linter for ConfigurationDriven
ClosedPublic

Authored by avivey on Oct 14 2014, 10:47 PM.
Tags
None
Tokens
"Love" token, awarded by vrusinov."Baby Tequila" token, awarded by joshuaspence."Manufacturing Defect?" token, awarded by chad.

Details

Summary

Support and prefer configuration from .arclint over Configuration Manager

Test Plan

arc lint with several combinations of values in .arcconfig and .arclint.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

avivey retitled this revision from to Modernize Script-and-Regex linter for ConfigurationDriven.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a reviewer: epriestley.
joshuaspence edited edge metadata.

This generally looks pretty good to me, some minor inlines.

src/lint/linter/ArcanistScriptAndRegexLinter.php
390–395

Change this to $config->getDeprecatedConfiguration('linter.scriptandregex.script')

391

Probably just use if ($this->script) { ... }

420

As above.

424–425

As above.

This revision now requires changes to proceed.Jan 6 2015, 11:20 AM
src/lint/linter/ArcanistScriptAndRegexLinter.php
390–395

ok.

391

if (strlen($x)) is the convention used, to assure $x is actually a string (@epriestley said so). Or at least it was.

424–425

$key is used in the error message below.

avivey edited edge metadata.
  • use getDeprecatedConfiguration
joshuaspence edited edge metadata.

We should maybe change the configuration keys to 'script-and-regex.script' and 'script-and-regex.regex' for consistency with other linters, but otherwise this looks good.

src/lint/linter/ArcanistScriptAndRegexLinter.php
299

We should eventually make this non-optional I guess.

303

As above.

@epriestley, just bumping this as I figured it has fallen off your radar.

joshuaspence edited edge metadata.

Just noticed a couple of things whilst working on D11773.

src/lint/linter/ArcanistScriptAndRegexLinter.php
11

We should update this.

14

We should update this.

298

Change this to script-and-regex.script.

302

Change this to script-and-regex.reged.

313

As above.

316

As above.

391–395

See line 413.

397–399

See line 420.

420

I still think that we should just use if ($this->regex) { here...

427–429

...or change this to be if (!strlen($config)) {

440–444

I'm thinking that we should just remove the second part of this message, since it encourages the use of deprecated configuration.

This revision now requires changes to proceed.Feb 15 2015, 8:29 PM

I wrote a long text here describing what I think the logic of strlen is, but it turns out that php.
I'll just remove the strlen for now, unless @epriestley want to chime in.

src/lint/linter/ArcanistScriptAndRegexLinter.php
299

Yeah, that's the plan, but I'd like to avoid breaking this so abruptly; Give it some time as "deprecated".

The use of strlen() to test whether strings are set is generally correct, because strings like "0" and "0.0" fail if ($setting) but pass if (strlen($setting)). These cases rarely matter, but occasionally they do things like prevent you from writing rules that affect files literally named "0", making users with the username "0" behave awkwardly, preventing you from making a wiki page titled "0", etc.

Ok, that makes more sense.
In the case of this diff, it can only make scripts named "0" not work, which is probably not so bad?

avivey edited edge metadata.

update config names, remove strlen

joshuaspence edited edge metadata.

LGTM otherwise.

src/lint/linter/ArcanistScriptAndRegexLinter.php
314

Maybe you should do Filesystem::assertExists to ensure that the script actually exists... or maybe this already happens somewhere else?

391–395

For consistency, maybe this if ($this->script) {

399–402

Maybe maybe this translatable with pht()

429–432

Make this translatable with this pht()

440–444

Prefer to write this as pht("%s: Regex '%s' does not compile.", __CLASS__, $regex)

epriestley edited edge metadata.

I'll fix that pht() stuff in the pull.

src/lint/linter/ArcanistScriptAndRegexLinter.php
314

This might be grep or some shell alias or something so I think it's OK to not check.

This revision is now accepted and ready to land.May 5 2015, 10:49 AM
src/lint/linter/ArcanistScriptAndRegexLinter.php
314

Ah, good point.

This revision was automatically updated to reflect the committed changes.