Paths

Table of Contentst

Modernize Script-and-Regex linter for ConfigurationDriven
ClosedPublic
Actions

Authored by avivey on Oct 14 2014, 10:47 PM.

Details

Reviewers

joshuaspence
epriestley

Group Reviewers

Blessed Reviewers

Commits

rARCc00899ad60ff: Modernize Script-and-Regex linter for ConfigurationDriven

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

s-re-lint

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 2833
Build 2837: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

avivey updated this revision to Diff 25700.Oct 14 2014, 10:47 PM

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.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptOct 14 2014, 10:47 PM

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

Harbormaster completed remote builds in B2833: Diff 25700.Oct 14 2014, 10:48 PM

joshuaspence added a reviewer: joshuaspence.Oct 15 2014, 1:55 AM

chad awarded a token.Oct 15 2014, 2:43 AM

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

src/lint/linter/ArcanistScriptAndRegexLinter.php
386	Probably just use `if ($this->script) { ... }`
390–393	Change this to `$config->getDeprecatedConfiguration('linter.scriptandregex.script')`
414	As above.
417–420	As above.

This revision now requires changes to proceed.Jan 6 2015, 11:20 AM

avivey added inline comments.Jan 13 2015, 9:16 PM

src/lint/linter/ArcanistScriptAndRegexLinter.php
386	`if (strlen($x))` is the convention used, to assure $x is actually a string (@epriestley said so). Or at least it was.
390–393	ok.
417–420	`$key` is used in the error message below.

use getDeprecatedConfiguration

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
294	We should eventually make this non-optional I guess.
298	As above.

joshuaspence mentioned this in D9084: Modernize the script and regex linter.Jan 19 2015, 7:41 AM

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

joshuaspence awarded a token.Feb 10 2015, 8:16 AM

joshuaspence added a child revision: D11773: [Draft] Convert `ArcanistScriptAndRegexLinter` to `ArcanistExternalLinter`.Feb 15 2015, 8:28 PM

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.
293	Change this to `script-and-regex.script`.
297	Change this to `script-and-regex.reged`.
308	As above.
311	As above.
393	See line 413.
395–396	See line 420.
414	I still think that we should just use `if ($this->regex) {` here...
422–423	...or change this to be `if (!strlen($config)) {`
433–434	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
294	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?

update config names, remove strlen

jpoehls added a subscriber: jpoehls.Apr 20 2015, 2:38 PM

vrusinov awarded a token.Apr 25 2015, 2:10 PM

vrusinov added a subscriber: vrusinov.

LGTM otherwise.

src/lint/linter/ArcanistScriptAndRegexLinter.php
309	Maybe you should do `Filesystem::assertExists` to ensure that the script actually exists... or maybe this already happens somewhere else?
393	For consistency, maybe this `if ($this->script) {`
396	Maybe maybe this translatable with `pht()`
423	Make this translatable with this `pht()`
432–434	Prefer to write this as `pht("%s: Regex '%s' does not compile.", __CLASS__, $regex)`