Summary:
FB runs some linters on background and it's a magnificent hack using `ParallelLinter` (two instances), `BackgroundLinter` and `FutureLinter`.
I want to simplify this by resolving the futures in engine instead of in some virtual linter.
It also seems like a better place to do it.
It should also fix caching problems I have with them (because the virtual linters don't know about the cache at all).
Test Plan: None yet.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4380
Summary: This fix lets you run arc lint from any directory in the repository
Test Plan: Ran arc lint from any directory
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4356
Summary:
Lints cpp code using the cppcheck static linter. This linter needs to
be downloaded and built from http://cppcheck.sourceforge.net/
Test Plan: Used it on a few files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4353
Summary: Adds arc lint support for cpp files with Google's cpplint.py lint checking.
Test Plan: ran it on some cpp files. Added unit tests
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4344
Summary: flake8 is the better maintained combination of pep8 and pyflakes
Test Plan: There's a test!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, jack
Differential Revision: https://secure.phabricator.com/D4082
Summary:
We can add `GRANULARITY_DIRECTORY` and `GRANULARITY_REPOSITORY` later.
Repository granularity may use current commit + changes.
Directory would need to use hashes of all files in dir which would be quite expensive.
Test Plan:
$ echo '<?php class A extends B {}' > A.php
$ arc lint --cache 1
$ arc lint --cache 1
$ echo '<?php class B {}' > B.php
$ arc lint --cache 1
$ arc lint --cache 1
$ rm B.php
$ arc lint --cache 1
$ arc lint --cache 1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4021
Summary:
This was causing the following error in environments that didnt have scala configured:
Some linters failed:
- ArcanistScalaSBTLinter: ArcanistUsageException: This directory does not appear to be maintained by SBT, as we can't seem to find a working build file (project/Build.scala or build.sbt).
Test Plan: .
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4070
Summary:
SBT is the most common Scala buildsystem. This adds an extremely basic and
slightly horrible linter to check SBT's output for warnings and errors.
Test Plan:
Tested this with a Scala project I've been working on for some time.
It seemed relatively sane.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4064
Summary:
- Rename some very old variables.
- Wrap some contributed lines.
Test Plan: `arc lint` / `arc unit`. Viewed a diff in an uncacheable mode to verify intraline behavior.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4018
Summary:
Let's try this.
It may be useful to see this together in Differential overview.
Test Plan: Linted a file with TODO.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T744
Differential Revision: https://secure.phabricator.com/D4014
Summary:
There is one big decision: How to get linters version.
I've created `getCacheVersion()` which is supposed to be bumped every time engine or any linter is changed.
This is not very nice but the other alternative (detect this automatically) seems worse:
- The engine may be outside repo and may or may not be under version control so getting its version through something like `git log` may not be even possible.
- If it is in the same repo then every rebase will obsolete the whole cache.
Even though bumping the version manually is PITA I still think it's a better solution.
Test Plan:
$ time arc lint --cache 1
# verified file
$ arc arc lint --cache 1
# added some debug output to see the cached results
# also observed better time (.57 s instead of 2.19 s)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2036
Differential Revision: https://secure.phabricator.com/D4006
Summary:
Installations extend this.
Another solution would be to extend `ArcanistLinterTestCase` from `ArcanistArcanistLinterTestCase` and return null in `getLink()` to avoid code duplication but I prefer clean class hierarchy.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3878
Test Plan:
$ arc lint a.py # with too long line
Reviewers: zeeg, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3882
Summary: Maybe I will need it on other places.
Test Plan:
$ arc lint --output json # on file with lint error
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3898
Summary:
Makes lots of noise:
{F22758}
Test Plan:
Linted file with several bad characters per line.
Linted file with one bad character per line.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3896
Summary: We could also inject the value from the test case config but this is simpler.
Test Plan:
$ arc unit src/lint/linter/ArcanistLicenseLinter.php
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3895
Summary: None of these are that serious that I would like to be informed about them on unmodified lines.
Test Plan: Linted Python file with lots of PEP8 errors, now warnings.
Reviewers: zeeg, epriestley
Reviewed By: zeeg
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3884
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in unit tests and LICENSE file.
Reviewers: epriestley, btrahan, edward
Reviewed By: epriestley
CC: aran, Korvin, davidrecordon
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3881
Summary: This information may be quite useful.
Test Plan: Threw exception from license linter.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3764
Summary:
This code is much more readable to me.
It should also be faster as building the array at once should be faster than one by one.
Test Plan: Made license and XHPAST errors in PHP file, made spelling error in JS file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3758
Summary:
Currently, ArcanisSingleLintEngine lints deleted paths and directories. These are sometimes appropriate, but SingleLintEngine is a less-sophisticated linter and should have more safe defaults.
Also fix an error where JSHint reported useless messages on failure.
Test Plan:
Reproduced the problem:
$ git show
commit d71efe2b13770c8861bcd3415c15503fc377339f
Author: epriestley <git@epriestley.com>
Date: Fri Oct 19 12:22:50 2012 -0700
WIP
diff --git a/test.js b/test.js
deleted file mode 100644
index 8bd6648..0000000
--- a/test.js
+++ /dev/null
@@ -1 +0,0 @@
-asdf
$ arc set-config --local lint.engine.single.linter ArcanistJSHintLinter
Set key 'lint.engine.single.linter' = "ArcanistJSHintLinter" in local config (was null).
$ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
Usage Exception: JSHint returned output we can't parse. Check that your JSHint installation.
Output:
Applied the error message fix:
$ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
Usage Exception: JSHint returned unparseable output.
stdout:
stderr:
node.js:181
throw e; // process.nextTick error, or 'error' event on first tick
^
Error: ENOENT, No such file or directory '/INSECURE/repos/git-working-copy/test.js'
at Object.statSync (fs.js:400:18)
at _collect (/usr/lib/node_modules/jshint/lib/hint.js:77:12)
at /usr/lib/node_modules/jshint/lib/hint.js:93:13
at Array.forEach (native)
at Object.hint (/usr/lib/node_modules/jshint/lib/hint.js:92:17)
at Object.interpret (/usr/lib/node_modules/jshint/lib/cli.js:137:21)
at Object.<anonymous> (/usr/lib/node_modules/jshint/bin/hint:2:25)
at Module._compile (module.js:420:26)
at Object..js (module.js:426:10)
at Module.load (module.js:336:31)
Applied the remove paths fix:
$ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
Usage Exception: No paths are lintable.
Reviewers: magazovski, btrahan, vrana
Reviewed By: btrahan
CC: aran, Korvin, vissi
Differential Revision: https://secure.phabricator.com/D3735
Summary:
When some linter throws then we don't print any result.
This is bad in case when the linter threw e.g. because of syntax error in some file which some other linter will tell us about.
Test Plan: Threw from a linter, made lint error in a file, saw error then exception.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3756
Summary: This also changes how we treat `@generated` and `@nolint` - after this diff, we will still lint their path.
Test Plan:
Created directory `a.php` and symlink `b.php` pointing to directory.
`arc lint` previously printed:
> Requested path `/data/users/jakubv/devtools/arcanist/_.php' is not a file.
Now it prints:
> No lint warnings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3737
Summary: I use it more often without second parameter than with it.
Test Plan:
Lint of:
preg_quote('');
preg_quote('', '/');
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3647
Summary: $param is null here. it should be $node.
Test Plan: arc lint no longer barfed on me!
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3540
Summary: I don't offer a replacement because `f() ?: 1` converted to `f() ? f() : 1` can cause side effects and whitespace issues.
Test Plan: New test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3489
Summary:
`pht()` can be some random function.
Better solution would be to separate this linter to its own class but it would be slower.
Also use `PhutilLintEngine` as `lint.engine`.
Test Plan:
$ arc lint # on file with pht($a)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3480
Summary: Add `ruby -wc` as a linter and raise Error whenever there's a syntax error
Test Plan: Just a few dumb unit tests.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3447
Test Plan:
Wrote "Posible" in the linter, let it change to "Possible".
New unit test.
Reviewers: jack, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3469
