Page MenuHomePhabricator

Use the `ArcanistConfigurationDrivenLintEngine` as a linting engine.
ClosedPublic

Authored by joshuaspence on May 12 2014, 2:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 24, 9:47 PM
Unknown Object (File)
Sun, Apr 21, 3:39 PM
Unknown Object (File)
Sun, Apr 21, 5:26 AM
Unknown Object (File)
Sat, Apr 20, 6:52 PM
Unknown Object (File)
Wed, Apr 17, 2:29 PM
Unknown Object (File)
Mon, Apr 15, 5:20 PM
Unknown Object (File)
Mon, Apr 15, 4:53 PM
Unknown Object (File)
Thu, Apr 11, 7:03 AM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP566f8ab9aa3a: Use the `ArcanistConfigurationDrivenLintEngine` as a linting engine.
Summary

Ref T2039. This diff is the equivalent to D9057, but for rP.

Depends on D9066.

Test Plan

Ran arc lint and ensure it doesn't complain about the .arclint file.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Use the `ArcanistConfigurationDrivenLintEngine` as a linting engine..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence added a task: Restricted Maniphest Task.
.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.

epriestley edited edge metadata.

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.

This revision now requires changes to proceed.May 12 2014, 2:56 AM
.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.

.arclint
40

Oh, of course... ArcanistPhutilXHPASTLinter is in rARC not in rP.

@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": {
        ...
      }
    } 
  }
joshuaspence edited edge metadata.

Fix path for raphael.

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.

joshuaspence edited edge metadata.

Namespace configuration options.

epriestley edited edge metadata.

This probably needs "phutil-library" too, but I'll just kick it in as-is to avoid creating too much reverse causality in history.

This revision is now accepted and ready to land.May 12 2014, 11:48 AM
epriestley updated this revision to Diff 21543.

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