Page MenuHomePhabricator
Differential D9089

Be more strict with the type of `.arclint` properties.
ClosedPublic
Actions

Authored by joshuaspence on May 13 2014, 8:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 6:48 PM
Unknown Object (File)
Mon, Dec 16, 11:07 PM
Unknown Object (File)
Sun, Dec 15, 7:08 AM
Unknown Object (File)
Wed, Dec 11, 1:22 AM
Unknown Object (File)
Fri, Dec 6, 5:30 AM
Unknown Object (File)
Thu, Dec 5, 12:05 PM
Unknown Object (File)
Wed, Dec 4, 9:33 PM
Unknown Object (File)
Wed, Dec 4, 9:33 PM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
rARCb744ed9a19ce: Be more strict with the type of `.arclint` properties.
Summary

Although this provides less context in terms of the error message (for example, Parameter has invalid type. Expected type 'optional regex|list<regex>', got type 'list<string>'.), I think that it is the right approach. I think that PhutilTypeSpec::checkMap should be improved such that additional context is provided in the exception message.

Test Plan

Ran arc lint. Modified .arclint to contain an invalid regex and ran arc lint again.

Diff Detail

Repository
rARC Arcanist
Branch
arclint-regex
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 417
Build 417: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 21595.May 13 2014, 8:49 AM
joshuaspence retitled this revision from to Be more strict with the type of `.arclint` properties..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 13 2014, 8:49 AM
Herald added subscribers: Korvin, epriestley. · View Herald Transcript
Harbormaster completed remote builds in B417: Diff 21595.May 13 2014, 8:50 AM
epriestley requested changes to this revision.May 13 2014, 8:14 PM
epriestley edited edge metadata.

Let's at least improve the error message on the easy one? I don't think PhutilTypeSpec can know about that.

src/lint/engine/ArcanistConfigurationDrivenLintEngine.php
65–71

Here, at least, we should be able to catch and throw a PhutilProxyException like below...

84

(Like this.)

This revision now requires changes to proceed.May 13 2014, 8:14 PM
joshuaspence added inline comments.May 13 2014, 9:50 PM
src/lint/engine/ArcanistConfigurationDrivenLintEngine.php
65–71

Sure, but how can we know which key was invalid?

epriestley added a comment.May 13 2014, 9:51 PM

Just raising the linter key would be helpful -- "There was an error in the configuration for linter "%s": ..." or similar

joshuaspence updated this revision to Diff 21641.May 13 2014, 9:57 PM
joshuaspence edited edge metadata.

Use PhutilProxyException to throw a more descriptive error message.

Harbormaster completed remote builds in B443: Diff 21641.May 13 2014, 9:58 PM
epriestley accepted this revision.May 13 2014, 10:17 PM
epriestley edited edge metadata.

good enough for government work

This revision is now accepted and ready to land.May 13 2014, 10:17 PM
epriestley closed this revision.May 13 2014, 10:18 PM
epriestley updated this revision to Diff 21642.

Closed by commit rARCb744ed9a19ce (authored by @joshuaspence, committed by @epriestley).

Revision Contents
Changeset List

PathSize
src/
lint/
engine/
36 lines

Diff 21595

View Options

src/lint/engine/ArcanistConfigurationDrivenLintEngine.php

Loading...
Phabricator · Built by Phacility