Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- Restricted Maniphest Task
- Commits
- Restricted Diffusion Commit
rP566f8ab9aa3a: Use the `ArcanistConfigurationDrivenLintEngine` as a linting engine.
Ran arc lint and ensure it doesn't complain about the .arclint file.
Diff Detail
- Repository
- rP Phabricator
- Branch
- arclint
- Lint
Lint Warnings Severity Location Code Message Warning .arclint:40 TXT3 Line Too Long - Unit
Tests Skipped - Build Status
Buildable 389 Build 389: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
.arcconfig | ||
---|---|---|
6–7 | Ideally, these would be moved into .arclint. I was going to do that separately, but if you ask nicely I will put it into this diff. |
One minor nitpick about the raphael path, and I'm not sure about the functions thing.
.arcconfig | ||
---|---|---|
6–7 | Haha, separately is fine. :P | |
.arclint | ||
16 | This doesn't actually exist anymore, it's in webroot/rsrc/externals/raphael/ now. Not this diff's fault, but might as well fix it. | |
25 | As above. | |
40 | This needs more code to work? We should probably namespace the option -- but we could also just remove the configuration and deal with this elsewhere, since there is only one remaining call to this function (phutil_escape_html()) anywhere in the codebase, so this warning has done its job, essentially. |
.arclint | ||
---|---|---|
40 | Ah whoops. I think that I didn't commit one of the files (ArcanistPhutilXHPASTLinter). I generally agree with you regarding removing this if it is basically not needed anymore. However, the deprecated functions concept is sort of neat and could have use outside of Phabricator, in which case moving it to ArcanistXHPASTLinter makes sense. |
@epriestley, with regards to "namespacing" linter configuration I'm ultimately happy with whatever, but is it really necessary to namespace the options? AFAIK, namespacing is effectively achieved by the type of the linter configuration.
i.e. In the following example, deprecated.functions is implied to be phutil-xhpast.deprecated.functions.
{ "linters": { "phutil-xhpast": { "type": "phutil-xhpast", "include": "(\\.php$)", "deprecated.functions": { ... } } }
I don't think it's strictly necessary, but, e.g., the C# linter (or something) adds an option called discovery. If we come up with some meaningful option called discovery later on that we'd like to put at the ExternalLinter or Linter levels, we'd be conflicting with the key in the subclass.
We'll probably never have a generic deprecated.functions, but something like discovery doesn't seem completely out of the question.
This also means that copy/paste issues will be caught immediately if someone transfers similar things between linters.
And .arclint should be written only very rarely, so it's not like we're extracting a high price in keystrokes...
I feel that a better approach would be to nest any linter-specific configuration under a common key rather than rely on the linter to choose a non-conflicting key.
For example,
{ "linters": { "phutil-xhpast": { "type": "phutil-xhpast", "include": "(\\.php$)", "config": { "deprecated.functions": { ... } } } }
However, if you'd prefer phutil-xhpast.deprecated.functions then I'll settle for that.
This probably needs "phutil-library" too, but I'll just kick it in as-is to avoid creating too much reverse causality in history.
Closed by commit rP566f8ab9aa3a (authored by @joshuaspence, committed by @epriestley).