Summary:
This allows engines to check the canRun method on linters,
which should determine if a linter is configured and can
be run in the current environment.
Test Plan:
Implement a linter with canRun as false, and ensure
it doesnt run.
Ensure all existing linters still run by default.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1445
Summary:
This adds a new lint engine, ComprehensiveLintEngine, which
includes sane defaults for some generic languages.
Ideally this would include *all* available language linters,
but that can be enhanced at a later point. Right now it's mostly
the base linter with additional JavaScript and Python linters.
Test Plan:
Adjust the lint_engine to be "ComprehensiveLinterEngine". You'll
also need jshint, pyflakes, and pep8 to all be available on PATH.
Run arc lint against files which contain .php, .py, and .js.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1439
Summary:
Inspired by http://news.ycombinator.com/item?id=3464671 and a lot of
diffs I've seen @ FB, I've added a spell checking linter. To reduce
false positives, it's only a blacklist. Still, it catches a large
number of 'issues'.
Test Plan:
Unit tests. Ran on FB's codebase. No false positives
noticed but a lot of cases caught.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, jack
Differential Revision: https://secure.phabricator.com/D1409
This patch adds ArcanistJSHintLinter class and three new .arcconfig options:
* lint.jshint.prefix - directory where JSHint binary resides
* lint.jshint.bin - JSHint binary name (if different from 'jshint')
* lint.jshint.config - a JSON file with JSHint project-wide options
By default, this linter assumes that JSHint is installed on user's system
as an NPM package.
Test Plan:
(1)
Run `npm install jshint -g` to install JSHint and add
ArcanistJSHintLinter to your Lint Engine (I didn't see PEP8 or
PyFlakes in the PhutilLintEngine so I decided to not to put
JSHint in there). After that you can do `arc lint my.js`.
(2)
Create a new file, config.json, and add `{ "white": true }` in it.
Add `"lint.jshint.config": "/path/to/config.json"` to your .arcconfig and
run `arc lint my.js`. After that, unless your name is Douglas Crockford,
you'll see tons of JSHint warnings about minor PEP8-like things (spacing, etc.)
Summary:
If the else clause does not have braces (one-liner), XHPASTLinter eats the
newline and brings the body statement of the else clause to the same line.
Test Plan: added a new test to space-after-control-keywords.lint-test
Reviewers: epriestley
CC: aran, epriestley, kiyoto
Differential Revision: https://secure.phabricator.com/D1367
Summary: See T645. These commands take inconsistent and overly-magical arguments
right now. Instead, make them behave consistently and allow them both to operate
on "arc <workflow> path path2 path3 ...", which is a generally useful workflow.
Test Plan: Ran "arc lint <path>", "arc unit <path>", "arc lint --rev
HEAD^^^^^^", "arc unit --rev HEAD^^^^^^^^^^^^", etc. Ran "arc diff --trace" and
verified --rev argument to child workflows.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T645
Differential Revision: https://secure.phabricator.com/D1348
Summary:
- Remove XHP tests so I can remove XHP support from XHPAST.
- Update license tests so they don't break every year.
Test Plan: - Ran all unit tests.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, pad, jungejason, btrahan
Maniphest Tasks: T635
Differential Revision: https://secure.phabricator.com/D1318
Summary: Previously, we would not correct missing space before "{".
Test Plan: Ran unit tests.
Reviewers: btrahan, jungejason, kiyoto
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1313
Summary: Previously, we would not correct excessive space after "if", etc.
Test Plan: Ran test case.
Reviewers: btrahan, jungejason, kiyoto
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: https://secure.phabricator.com/D1311
Summary:
Creates a new hook API that can be used to interface with
SVN/Git/Mercurial in the context of a commit hook. Currently only adds a
function to read the modified file data in a Subversion commit hook.
An object of this API is created in the SvnHookPreCommitWorkflow and
passed on the Lint Engine which then uses it to access current file
data, of the way the APIs seem to be structured); linters use the
getData function which is essentially a wrapper around the engine's
call, with another layer of caching.
Task ID: #770556
Blame Rev:
Test Plan:
- Create a local svn repository and add a minimal hook to run the local
version of arc to test commits
(http://phabricator.com/docs/arcanist/article/Installing_Arcanist_SVN_Hooks.html)
- Create a temporary repository that can trigger any of the linters
available, and test against a temporary linter by committing against
the test repository: the linter should be able to access all required
files by using loadData/getData in the LintEngine and Linter.
Revert Plan:
Tags: lint, svn-hook-pre-commit
Reviewers: jungejason, asukhachev, epriestley, aran
Reviewed By: epriestley
CC: aran, jungejason, epriestley, kunalb, asukhachev
Differential Revision: https://secure.phabricator.com/D1256
Summary: --output json will be used for scripts, so support script writers a
little better.
Test Plan:
arc lint [--output {json, summary}]
arc lint --output json {--apply-patches, --never-apply-patches}
Reviewers: epriestley
Reviewed By: epriestley
CC: jack, aran, epriestley
Maniphest Tasks: T675
Differential Revision: 1220
Summary:
Adds a 'desc' field to json output so I can use that inside my vim
plugin.
Test Plan:
Lint a few things. Description text shows up in vim
Reviewers: epriestley
Reviewed By: epriestley
Differential Revision: 1210
Summary:
add method setXHPASTTreeForPath() so that a child class of
ArcanistXHPASTLinter can set the tree easily.
Test Plan: wrote a subclass of ArcanistXHPASTLinter and it worked.
Reviewers: pad, epriestley
Reviewed By: pad
CC: aran, pad, jungejason
Differential Revision: 1134
Summary: if we don't care and don't expect it to break all other linters then
continue
Test Plan: ##arc lint --trace## on a commit with non-ascii characters, saw other
linters continue
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, cpiro
Differential Revision: 1122
Summary:
D1061 introduced a 'text file' check, but it fails under SVN for new
directories.
- Revert D1061 (This reverts commit b2cd18252701be2093b52652fb3d1d94c5df571e.)
- Make getChangedLines() return null to indicate that the operation doesn't
make sense. I think this was the intent of the code in the lint engine.
- Fix a bug where running "arc lint" on a change in an SVN working copy from a
subdirectory would fail.
- Fix a bug where warnings with no line information were incorrectly
discarded.
Test Plan:
- Ran "arc lint" in an SVN working copy with a new directory (no failure).
- Forced FilenameLinter to always raise a warning. Added a binary file and ran
"arc lint". The warning was reported for the new binary file, a new text file,
and a new directory.
Reviewers: jungejason, andrewjcg, nh, tuomaspelkonen, aran
Reviewed By: andrewjcg
CC: aran, andrewjcg, epriestley
Differential Revision: 1076
Summary: newer PyLint includes commas and a column number for every message
after the line number. ignore that if it's present.
Test Plan: ##arc lint --trace --lintall## a file with messages with the old and
new pylint (2.5), works fine with both
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1062
Summary:
Currently, lint messages are only included if they are errors or
if they affect lines which the diff changed. The implementation
of this caused issues for non-text files (e.g. binaries), as line
change information is not available, and the corresponding lint
messages were dropped (for non-errors).
In this diff, only lint messages concerning text files are dropped
based on this line filtering.
Test Plan: arc lint with binary file
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, aravindn, andrewjcg, liat, epriestley
Differential Revision: 1061
Summary: allow adding PYTHONPATHs directly in addition to the hardcoded few
already allowed.
Test Plan: ##arc lint## successfully picks up the paths listed in my .arcconfig,
and no longer errors on modules not found from those directories
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1050
Summary: add the ability to specify an rcfile path either absolute or relative
to the project root.
Test Plan: added one for a project, ran ##arc lint --trace## to see the expected
result
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1046
Summary:
- The top documentation is more readable, and the details about the severity
mapping regexps is moved to the top from inline -- they're important.
- array_merge() is used when appending command line args from
##lint.pylint.options## instead of '+'. The latter merges by key instead of
appending the two lists as intended.
- rely on the exit code instead of magic text that may or may not be there
depending on how ##pylint## is invoked.
These bugs were introduced in d762311a9d.
Test Plan: ##arc lint --trace##'d a bunch of Python source files in a project,
with and without PyLint messages. Added some ##lint.pylint.options## to see
proper appending behavior.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, cpiro
Differential Revision: 1045
Summary:
See T326. Allow lint rules to be selectively overridden, e.g. for Conduit
methods.
Since FB has a long history of suggesting crazy patches for this stuff I think
we're safer just adding a hook class than trying to do some kind of regexp
magic.
Test Plan: Wrote a hook for Phabricator and linted some Conduit files without
issues. Ran unit tests.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 874
Summary:
The module analyzer reads "phutil_require_module" in the source of a module as a
dependency, and tries to regenerate __init__.php if symbols from that module
aren't actually used. This creates patches which don't actually resolve the
problem, since changing __init__.php won't change the dependency.
Instead, trust that anyone using phutil_require_module in the source of a module
knows what they're doing and don't mark it as a dependency.
We currently have an issue with this in phabricator's Setup process since I load
some other libraries' modules just to test if they can be loaded
@lesha, this might be the issue you reported a while ago.
Test Plan: Ran "arc lint" on a module which pulls in another module explicitly
in the source, didn't get a no-op lint error.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, lesha
CC: aran, epriestley, jungejason
Differential Revision: 770
Summary: See D753. Let's just have lint fix this. Depends on D754 (it fails to
detect all the blocks without that patch, but doesn't do anything bad).
Test Plan: Ran unit tests.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: hunterbridges, aran, epriestley, jungejason
Differential Revision: 755
Summary:
In hphp's version of idx, it uses debug_rlog() which is not
available in phabricator's code. It will be executed if null is passed
as the second value to idx.
Test Plan:
ran arc lint which still report error message, and php
./scripts/arcanist.php unit src/lint/linter/xhpast/__test__ doesn't
throw.
Reviewed By: epriestley
Reviewers: tuomaspelkonen, epriestley
CC: aran, jungejason, epriestley
Differential Revision: 666
Summary:
Document the relationship between lint engines and linters. Provide an example
linter. Improve the documentation of PyLintLinter, which has a bunch of
configuration stuff which you had to dig into the code to get.
Test Plan:
Ran "arc lint --engine ExampleLintEngine --lintall derp.py" on a file with a
Python syntax error in it. Read documentation.
Reviewed By: j3kuntz
Reviewers: j3kuntz, andrewjcg
CC: aran, j3kuntz
Differential Revision: 557
Summary:
W293 reports a lint warning for the same problem that TXT6 reports a
lint error for. This isn't so terrible in and of itself.
However, when you are then prompted to apply a patch to fix TXT6, lint
doesn't realize that the W293 warning was also handled, and still
prompts you about ignoring unresolved warnings. This is misleading.
Test Plan:
> python pep8.py hasBlankLineWS.py
hasBlankLineWS.py:3:1: W293 blank line contains whitespace
> python pep8.py --ignore=W293 hasBlankLineWS.py
(no output)
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley, aran
Differential Revision: 525
Summary:
cpojer fixed a JS instance of this in D481, but we can catch it in PHP with
XHPAST. Add a lint rule to fail if nested loops use the same iterator.
Test Plan:
Ran unit test.
Reviewed By: tomo
Reviewers: aran, pad, cpojer, jungejason, tuomaspelkonen, tomo
Commenters: cpojer
CC: aran, cpojer, epriestley, tomo
Differential Revision: 489
Summary:
We have some "@generated" files of these types now, hit them with the text
linters
Test Plan:
Ran 'arc lint' on D444
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 451
Summary:
This is pretty coarse and could be refined, but I often do this when testing:
lang=diff
- if (some_complicated_condition())
+ if (true || some_complicated_condition())
aran has caught me not only doing it but sending out diffs with it like 30
times. Catch it in lint instead.
Test Plan:
Unit test, added a "true || $junk" to the code and linted it.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, pad
CC: aran
Differential Revision: 447
Summary:
Raise an error if an array is initialized with the same key present more than
once.
Test Plan:
bin/arc unit
Also added some duplicates to ArcanistXHPASTLinter.php and verified the output
of bin/arc lint.
Reviewed By: epriestley
Reviewers: jungejason, epriestley, aran
CC: aran, epriestley
Revert Plan:
Tags:
Differential Revision: 346
Summary:
Provides a lint class as a wrapper around the external project PyLint.
This exposes some arc config variables to control the behavior:
lint.pylint.prefix - non-standard installation location of pylint
lint.pylint.logilab_astng.prefix - non-standard installation location
of logilab-astng, a dependency of pylint
lint.pylint.logilab_common.prefix - non-standard installation location
of logilab-common, a dependency of pylint
lint.pylint.codes.{error,warning,advice} - regexes matching against
PyLint message codes which should trigger arc errors/warnings/advice
lint.pylint.options - options to pass PyLint
Test Plan:
used to lint python code
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 343
Summary:
Provide a simple linter wrapper around pyflakes. This relies on finding
pyflakes via:
- lint.pyflakes.path - arcconfig setting of absolute path to pyflakes
- lint.pyflakes.prefix - arcconfig setting of the prefix that pyflakes
was installed under
- users path
Test Plan:
linted python code with PyFlakes warnings
Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: aran, epriestley, andrewjcg, jungejason
Differential Revision: 310
Summary:
This diff:
- Adds the PEP8 linter to the externals directory
- Changes the path for finding pep8.py
- Removes use of execx since pep8.py return an errors code
when it finds PEP8 violations
Test Plan:
tested linting python code
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 309
Summary:
The story for creating and maintaining libphutil libraries and modules
is pretty terrible right now: you need to know a bunch of secret scripts and
dark magic. Provide 'arc liberate' which endeavors to always do the right thing
and put a library in the correct state.
Test Plan:
Ran liberate on libphutil, arcanist, phabricator; created new
libphutil libraries, added classes to them, liberated everything, introduced
errors etc and liberated that stuff, nothing was obviously broken in a terrible
way..?
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 269
Summary:
This isn't necessarily a root-cause solution but this syntax is really really
bad. If we want to fix the root cause, I'd recommend making its use a lint
error?
Test Plan:
Unit tests failed before patch, passed afterward.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 270
Summary:
This is necessary for the Javelin linters. The libphutil and flib linters do it
implicitly, so there's no real tradeoff here.
Test Plan:
Ran javelin linters.
Reviewed By: tuomaspelkonen
Reviewers: tomo, aran, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 238
Summary:
Declare victory like it's the United States.
Test Plan:
www copyright, apache copyright, control keyword whitespace and trailing
whitespace lint warnings
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 117
Summary:
Refactor ArcanistLintRenderer into three independent classes, and let
the Workflow select between them based on its 'output' parameter.
Test Plan:
Introduce a Lint warning and lint with no --output, with --output summary and
with --output JSON.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 96
Summary:
This is realistically always wrong and the author means "."
Test Plan:
lint / unit
Reviewed By: crackerjack
Reviewers: crackerjack, aran
CC: crackerjack
Differential Revision: 68
Summary:
XHPAST encounters a parse-depth problem on some files because PHP
5.2 has an un-overridable parse depth limit for JSON. The text of this error
says it is a "warning" but we currently raise an error. Make it a warning
instead.
The JSON depth is 20 until PHP 5.2.3, where it becomes 128. After PHP 5.3
it defaults to 512 and is user-configurable, which will allow us to resolve
this issue in nearly all cases.
Since I made if/else express as a list in the AST, this only actually arises
in long binary chains, most commonly string concatenation, like:
$out = 'a'.'a'.'a'.'a'...
...where each string is a variable or HTML tag and the program is constructing
a complicated document.
At some point I'll add some PHP 5.3 massaging to the XHPAST decoder itself to
raise this limit to something more huge.
Test Plan:
Ran "arc lint --lintall" on a file with a very deep binary
expression tree ("1 + 1 + 1 ...") and received a warning instead of an error.
Reviewed By: aran
Reviewers: pad, aran
CC: epriestley, aran
Differential Revision: 54
