Page MenuHomePhabricator

Modernize Script-and-Regex linter for ConfigurationDriven
ClosedPublic

Authored by avivey on Oct 14 2014, 10:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 1:47 PM
Unknown Object (File)
Mon, Mar 25, 1:47 PM
Unknown Object (File)
Feb 8 2024, 10:56 PM
Unknown Object (File)
Feb 3 2024, 1:07 AM
Unknown Object (File)
Feb 1 2024, 8:31 PM
Unknown Object (File)
Feb 1 2024, 6:15 PM
Unknown Object (File)
Feb 1 2024, 5:51 PM
Unknown Object (File)
Feb 1 2024, 1:33 PM
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
Branch
arcpatch-D10704
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 4828
Build 4844: [Placeholder Plan] Wait for 30 Seconds

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
387–392

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

388

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

413

As above.

417–418

As above.

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

ok.

388

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

417–418

$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
296

We should eventually make this non-optional I guess.

300

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.

295

Change this to script-and-regex.script.

299

Change this to script-and-regex.reged.

310

As above.

313

As above.

388–392

See line 413.

394–395

See line 420.

413

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

420–421

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

430–431

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
296

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
311

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

388

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

395

Maybe maybe this translatable with pht()

421

Make this translatable with this pht()

430–431

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
311

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
311

Ah, good point.

This revision was automatically updated to reflect the committed changes.