Summary: To me, it seems that these methods should never be overwritten in subclasses.
Test Plan: `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7958
Summary: Ref T2039. The type specs are right in theory but not quite correct in practice, since we pass strings in rather than objects.
Test Plan: While tweaking `phabricator/`, adjusted these to get desirable results.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9071
Summary: Ref T2039. It looks like "phutil-library" didn't make it over, I'll add that to the other two (unless I'm wrong and this isn't an oversight?).
Test Plan: Used `arc linters` to read help.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9070
Summary: It seems that there is a lot of overlap between `getConfig` / `setConfig` and `getLinterConfigurationOptions` / `setLinterConfigurationValue` respectively.
Test Plan: `arc lint` and `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9067
Summary: Currently, `PhabricatorLintEngine` configures the `ArcanistPhutilXHPASTLinter` linter. In order to use `.arclint` instead, we need to expose the `ArcanistPhutilXHPASTLinter` configuration to the `.arclint` format.
Test Plan: See D9064.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9066
Summary: Ref T2039. Make the `ArcanistCppcheckLinter` compatible with `.arclint`.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9068
Summary:
Ref T2039. I'll update the corresponding documentation.
It feels a little awkward that this is disconnected from `getLinterConfigurationOptions()`, but I dislike returning weird ad-hoc structures more than I dislike having two methods. Most linters don't implement either of these anyway.
Test Plan: Ran `arc linters` and `arc linters --verbose`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9062
Summary: Ref T2039. The `.arclint` file is reasonably complete now and we should start using it if possible, since we are trying to recommend it to others.
Test Plan: `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9057
Summary:
Currently, the Phutil XHPAST Linter and vanilla XHPAST Linter reuse the same parse tree, but do this by having explicit knowledge of one another.
Instead, let them synchronize by writing to a glorified array of globals on the Engine. They no longer require knowledge of one another, so this can work under `.arclint`.
(This could probably be a little cleaner by putting more logic in the shared base class, but Facebook has some kind of goofy subclass of this thing and //this// patch won't disrupt it, while a cleaner one might.)
This should unblock D9057.
Test Plan: Ran unit tests and normal lint, got accurate looking results without duplicate invocations showing up in `--trace`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9059
Summary: The `ArcanistGeneratedLinter` and `ArcanistNoLintLinter` don't actually ever raise any linter messages, so it doesn't make sense to set custom severities for these linters. Instead, don't expose this configuration.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9038
Summary:
Ref T2039. Addresses two issues:
- Issues a warning for use of config which is deprecated by `.arclint`.
- We no longer require an engine to be present for these linters, so `arc linters` doesn't fatal if they aren't configured.
Test Plan:
- Ran `arc linters` in a repo with no JSHint.
- Ran `arc linters` in a repo with junk in .arcconfig and got a warning when it was read. Verified it still took effect.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9058
Summary: Not all linters run on a `command file` fashion. In particular, the maven checkstyle plugin runs like `command --flag=file`.
Test Plan: Run a linter that extends `ArcanistExternalLinter`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9026
Summary: Ref T2039. This isn't exhaustive, but moves things forward by a decent chunk.
Test Plan: Used `arc linters` and read the messages.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9056
Summary: Ref T2039. We're starting to get kind of a lot of linters; provide `arc linters` to help users review and understand them and construct `.arclint` files.
Test Plan: {F152205}
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9041
Summary: Explicitly specify the types of the function parameters. This change is basically the same as D8388.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9040
Summary: The only real change here is adding a `getLinterConfigurationName` method so that this linter can be used with an `.arclint` file. Everything else is just some minor tidying.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9039
Summary: This isn't really necessary, nor does it bear any significant benefits, however semantically it seems to make sense. From `csslint --help`, the `--quiet` flag causes `csslint` to "Only output when errors are present".
Test Plan:
Tested using `csslint` directly:
```
> csslint test.css
csslint: No errors in /home/joshua/workspace/github.com/facebook/arcanist/foo.css.
> csslint --quiet test.css
```
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9036
Summary: No subclass should need to override these methods. Additionally, none currently do.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9035
Summary: PHPCS does actually support reading data from stdin, so let's make `ArcanistExternalLinter` aware of this.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9034
Summary: Convert the `cpplint.py` wrapper linter to `ArcanistExternalLinter`. This is in preparation for T2039.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9033
Test Plan:
Verified manually. Something like this in .arclint:
"linters" : {
"text" : {
"type" : "text",
"include" : "(\\.(txt|py|html?)$)",
"text.max-line-length": 200
}
changes the line length. Something other than an integer there raises an error.
Reviewers: epriestley, #blessed_reviewers, #arcanist
Reviewed By: epriestley, #blessed_reviewers, #arcanist
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D9029
Summary: The `lessc` binary is a part of the NPM `less` module.
Test Plan: `npm install less` works whereas `npm install lessc` does not.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9006
Summary: This class provides an adapter for [[https://github.com/less/less.js/ | lessc]].
Test Plan: Wrote and executed unit tests.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8992
Summary:
It seems that in some situations, JSHint does not set the `evidence`
property. In such cases, PHP fails with `Undefined property:
stdClass::$evidence`. It would be safer to access the error object as an
associative array rather than as `stdClass`.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8993
Summary: This linter is a wrapper around [[http://puppet-lint.com/ | puppet-lint[]].
Test Plan: Wrote an executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8990
Summary: These methods are declared `public`, but there are meant to be `protected` (as they are declared in `ArcanistExternalLinter`).
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8991
Summary: Add a linter which uses [[http://php.net/simplexml | SimpleXML]] to detect errors and potential problems in XML files.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8989
Summary: Provide bindings for [[https://github.com/zaach/jsonlint | JSONLint]], which is a useful tool for linting and validating JSON. Theoretically, this could be done with pure PHP, however it would not be trivial (`json_decode`, for example, does not provide any context as to JSON validation errors).
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8988
Summary:
This unit test is failing for me. It seems that I have a newer version of `flake8`. We should probably provide support for the most recent versions of any external tools, at least until we can implement version-specific stuff.
I am running `2.1.0 (pep8: 1.5.6, pyflakes: 0.8.1, mccabe: 0.2.1) CPython 2.7.6 on Linux`.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8987
Summary:
Ref T3186. Ref T2039.
- Convert JSHint to modern format; improve granularity of errors.
- Convert PyFlakes to modern format;
- Remove ApacheLicenseLinter and LicenseLinter (these have been deprecated for a very long time).
This is somewhat disruptive and will break some users by no longer respecting various path/config options. I'll sequence documentation and deprecation warnings in front of these.
Test Plan: Ran unit tests.
Reviewers: btrahan, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, joshuaspence, aran
Maniphest Tasks: T3186, T2039
Differential Revision: https://secure.phabricator.com/D6810
Summary:
This method will, theoretically, allow `arc lint` to be configured to require some minimum version of an external linter (although this would probably require significantly more work).
Additionally, the existence of this method simplifies the `getCacheVersion` function which, previously, was implemented by the external linters individually. Instead, a general approach to determining the version for cacheing purposes can be used.
Fixes T4954.
Test Plan: I'm not sure how to test this.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D8971
Summary: Modernize `ArcanistJSHintLinter` by extending from `ArcanistExternalLinter` instead of `ArcanistLinter`.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8965
See: <https://github.com/facebook/arcanist/pull/162>
It looks like the word `argument` appears many more times than `augment`, so I'm assuming `agument` is most likely to be a typo of `argument`.
Reviewed by: epriestley
Summary: Personally, I prefer to specify command lines flags as an array rather than a string.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aran, epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8387
Summary:
Fixes T4570. When a test case doesn't make any assertions, fail it:
- A tiny fraction of tests pass by not throwing. These tests can easily make a trivial assertion. It took about 5 minutes to fix them all (D8435, D8436).
- In other cases, no assertions means a test construction problem, as with T4570. In these cases, failing loudly catches a severe error.
- Fixes the no-assertion test cases in `arcanist/`
- Makes the PHP 5.4 test pass for the moment, see discussion in T4334.
Test Plan: Ran `arc unit --everything`.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8437
Summary: Ref T4570. Testing a directory with no recognized tests currently passes, but should fail.
Test Plan:
- Ran `arc unit`.
- Removed tests from a test directory, ran `arc unit`, got test failure.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8434
Summary: Allow `@todo` comments to be linted as TODOs as well as `TODO` comments.
Test Plan: I added a new test case (`todo.lint-test`)
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8389
Summary:
- Tidied up `ArcanistCSSLintLinter::getDefaultBinary`.
- Tidied up `CSSLintLinter::getDefaultFlags` function.
- Tidied up `ArcanistPhpcsLinter::getDefaultBinary` function.
- Tidied up `ArcanistPEP8Linter::getDefaultFlags` function
- Tidied up `ArcanistFlake8Linter::getDefaultFlags`.
- Tidied up `ArcanistCppcheckLinter::getLintOptions`.
- Tidied up `ArcanistCppcheckLinter::getLintPath`.
- Tidied up `ArcanistCpplintLinter::getLintOptions`.
- Tidied up `ArcanistCpplintLinter::getLintPath`.
- Removed child functions which are identical to the corresponding parent functions.
Test Plan: `arc lint` and `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8385
Summary: To me, it seems that these comments add no value. Personal opinion I suppose.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8382
Summary: See <https://github.com/facebook/arcanist/pull/131>
It looks like the behaviour of csslint has changed since the ArcanistCSSLintLinter was written. Consequently, ArcanistCSSLintLinter does not work with the latest version of csslint (v0.10.0).
- `csslint` has a non-zero exit status
- Fixed `csslint` parsing for v0.10.0
Reviewed by: epriestley
Summary:
Some callpaths end up here, which doesn't cause the internal linter's willLintPath() method. This can mean its `activePath` is set wrong, which results in us raising lint for the wrong file.
Particularly, if you apply D7979 (diff 18069) and `arc lint` it, you'll get a syntax error message in the wrong file.
Test Plan: Applied D7979 and linted it, got proper error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aarwine, aran
Differential Revision: https://secure.phabricator.com/D7988
