Page MenuHomePhabricator

Add external-json linter
AbandonedPublic

Authored by bgamari on Nov 22 2015, 8:04 PM.

Details

Summary

This provides a facility for implementing external linters which can
report violations via a JSON representation.

Test Plan

Try it

Diff Detail

Branch
master
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 8980
Build 10544: arc lint + arc unit

Event Timeline

bgamari updated this revision to Diff 35161.Nov 22 2015, 8:04 PM
bgamari retitled this revision from to Add external-json linter.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
epriestley requested changes to this revision.Nov 22 2015, 9:05 PM
epriestley added a reviewer: epriestley.
This revision now requires changes to proceed.Nov 22 2015, 9:05 PM
bgamari updated this revision to Diff 35169.Nov 22 2015, 10:23 PM
bgamari edited edge metadata.

Correct comments

epriestley requested changes to this revision.Nov 22 2015, 10:40 PM
epriestley edited edge metadata.

Per T9827, I currently believe this would be used by only one install and don't want to bring it upstream without seeing much broader interest first.

This revision now requires changes to proceed.Nov 22 2015, 10:40 PM

This code looks good to me in a technical sense, we just don't want to take over maintaining it. Two minor suggestions:

  • phutil_utf8_strtolower() has better behavior than strtolower() on some utf8 inputs.
  • phutil_json_decode() has better behavior (throws a detailed exception) on invalid JSON than json_decode() (returns false).

We don't currently have a way to, say, sign an extension as "this code is broadly reasonable and won't root your box or destroy your data, just not something the upstream wants to maintain or take responsibility for", but we might after T5055.

This code looks good to me in a technical sense, we just don't want to take over maintaining it. Two minor suggestions:

  • phutil_utf8_strtolower() has better behavior than strtolower() on some utf8 inputs.
  • phutil_json_decode() has better behavior (throws a detailed exception) on invalid JSON than json_decode() (returns false).

Thanks for the review!

We don't currently have a way to, say, sign an extension as "this code is broadly reasonable and won't root your box or destroy your data, just not something the upstream wants to maintain or take responsibility for", but we might after T5055.

Sure. I'll be putting the code up here.

bgamari abandoned this revision.Nov 22 2015, 11:09 PM