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

arcpatch-D10704

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 3825
Build 3837: [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
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

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

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.

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
296	We should eventually make this non-optional I guess.
300	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.
295	Change this to `script-and-regex.script`.
299	Change this to `script-and-regex.reged`.
310	As above.
313	As above.
388	See line 413.
394	See line 420.
413	I still think that we should just use `if ($this->regex) {` here...
420	...or change this to be `if (!strlen($config)) {`
431–432	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?

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
311	Maybe you should do `Filesystem::assertExists` to ensure that the script actually exists... or maybe this already happens somewhere else?
388–392	For consistency, maybe this `if ($this->script) {`
395	Maybe maybe this translatable with `pht()`
421	Make this translatable with this `pht()`
430–432	Prefer to write this as `pht("%s: Regex '%s' does not compile.", __CLASS__, $regex)`