Page MenuHomePhabricator
Differential D10773

Fix linter config check
ClosedPublic
Actions

Authored by avivey on Nov 2 2014, 2:25 AM.
Tags
None
Referenced Files
F14081109: D10773.diff
Fri, Nov 22, 6:27 PM
F14079781: D10773.diff
Fri, Nov 22, 9:48 AM
F14079616: D10773.diff
Fri, Nov 22, 9:07 AM
Unknown Object (File)
Sun, Nov 17, 10:37 PM
Unknown Object (File)
Thu, Nov 14, 5:18 AM
Unknown Object (File)
Sun, Nov 10, 3:44 AM
Unknown Object (File)
Wed, Nov 6, 12:46 PM
Unknown Object (File)
Fri, Nov 1, 2:52 PM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
joshuaspence
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T6311: 'arc linters' no longer reports linters as 'Configured', all show as 'Available'
Commits
rARC6f7bedaceb68: Fix linter config check
Summary

Fixes T6311 and T5124 by returning all configured linters from buildLinter(), and making ArcanistExternalLinter::checkBinaryConfiguration() not crash if there's no executable to run.

Test Plan

arc linters in rP shows "Configured" and "ERROR" as appropriate; Adding a broken linter to .arclint in rARC doesn't invoke it's not actually needed, and prints error if it is.

Diff Detail

Repository
rARC Arcanist
Branch
arcpatch-D10773
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 3295
Build 3302: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

avivey updated this revision to Diff 25858.Nov 2 2014, 2:25 AM
avivey retitled this revision from to Fix linter config check.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added reviewers: Blessed Reviewers, joshuaspence.
avivey added a task: T6311: 'arc linters' no longer reports linters as 'Configured', all show as 'Available'.
Herald edited edge metadata. · View Herald TranscriptNov 2 2014, 2:25 AM
Herald added a subscriber: epriestley. · View Herald Transcript
Harbormaster completed remote builds in B2931: Diff 25858.Nov 2 2014, 2:26 AM
avivey added a comment.Nov 2 2014, 2:27 AM

I initially attempted to implement canRun(), but if it's false, the lint engine just ignores the linters. I think it should fail the lint instead.

chad edited edge metadata.Dec 11 2014, 1:52 AM
joshuaspence requested changes to this revision.Dec 17 2014, 8:32 PM
joshuaspence edited edge metadata.
joshuaspence added inline comments.
src/lint/linter/ArcanistExternalLinter.php
358

I think that returning null would make more sense.

This revision now requires changes to proceed.Dec 17 2014, 8:32 PM
avivey updated this revision to Diff 26453.Dec 19 2014, 1:03 AM
avivey edited edge metadata.

yeah, probably :)

joshuaspence requested changes to this revision.Dec 30 2014, 9:07 AM
joshuaspence edited edge metadata.

I think you missed something when you updated the diff? It looks exactly the same to me.

This revision now requires changes to proceed.Dec 30 2014, 9:07 AM
joshuaspence added a reviewer: epriestley.Jan 3 2015, 1:11 PM
avivey added a comment.Jan 3 2015, 8:53 PM

Yes, it seems I have forgotten how to arc :p

Updating again...

avivey updated this revision to Diff 26838.Jan 3 2015, 8:58 PM
avivey edited edge metadata.

replace 'n/a' with null

Herald added a subscriber: Korvin. · View Herald TranscriptJan 3 2015, 8:58 PM
joshuaspence accepted this revision.Jan 3 2015, 11:12 PM
joshuaspence edited edge metadata.

LGTM

src/lint/linter/ArcanistExternalLinter.php
355–359

Since this looks a little out-of-place, I feel that we should comment it.

357

Just a general remark, I really feel that we overuse ArcanistUsageException.

joshuaspence updated this object.Jan 3 2015, 11:13 PM
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence edited edge metadata.
epriestley edited edge metadata.Jan 4 2015, 4:40 PM

D11205 changes the exception raised here.

avivey planned changes to this revision.Jan 4 2015, 9:25 PM

I'm not sure this issue still exists any more.

joshuaspence added a comment.Jan 4 2015, 9:26 PM
In D10773#103450, @avivey wrote:

I'm not sure this issue still exists any more.

T6311 still exists for me.

avivey requested a review of this revision.Jan 4 2015, 9:40 PM

I was trying to update it to use ArcanistUsageException, and didn't notice how D11205 hasn't landed yet, and things got weirder from there.

Like, php is fine with trying to catch an exception class that was never defined.

OTOH, once the exception we're catching there is ArcanistUsageException, the whole try-catch-checkBinaryConfiguration block is readable enough, IMO. I'll update it, once it's actually landed...

avivey updated this revision to Diff 27084.Jan 8 2015, 4:26 AM
avivey edited edge metadata.

check for ArcanistMissingLinterException

joshuaspence accepted this revision.Jan 8 2015, 8:02 AM
joshuaspence edited edge metadata.

awesometown

joshuaspence added a comment.Jan 9 2015, 10:43 PM

@epriestley, are you okay with this? This blocks T6061.

joshuaspence added a comment.Jan 19 2015, 7:51 PM

Bumping this seeing as it fixes T6311: 'arc linters' no longer reports linters as 'Configured', all show as 'Available'.

avivey marked 3 inline comments as done.Apr 24 2015, 7:06 PM
avivey added inline comments.
src/lint/linter/ArcanistExternalLinter.php
355–359

I think that narrowing the caught exception here resolves these two.

avivey marked an inline comment as done.Apr 24 2015, 7:09 PM
joshuaspence added a comment.May 5 2015, 8:58 PM

@epriestley, it would be great to land this. Without this fix, arc linters is a bit broken.

epriestley accepted this revision.May 5 2015, 9:02 PM
epriestley edited edge metadata.

shipitquick

This revision is now accepted and ready to land.May 5 2015, 9:02 PM
Closed by commit rARC6f7bedaceb68: Fix linter config check (authored by avivey, committed by epriestley). · Explain WhyMay 5 2015, 9:03 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
lint/
engine/
6 lines
linter/
5 lines

Diff 26453

View Options

src/lint/engine/ArcanistConfigurationDrivenLintEngine.php

Loading...
View Options

src/lint/linter/ArcanistExternalLinter.php

Loading...
Phabricator · Built by Phacility