Summary:
Ref T3186. Brings another linter onboard. This one uses the stdin stuff.
The unit test was ostensibly broken so I fixed it, but that might just be some kind of version issue.
Test Plan: Unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6802
Summary:
Ref T3186. We have about 50 linters which run programs and read the results, all of which have ad-hoc one-off custom config that isn't formalized anywhere.
Consolidate all this stuff into `ArcanistExternalLinter`, which is configurable through `.arclint` (although nothing supports this quite yet).
Extend CSSLint and Pep8Lint from `ArcanistExternalLinter`.
Add unit tests for both.
There are still some rough edges here, but it mostly seems to work pretty well.
Test Plan: Ran unit tests, hit some (most?) of the error cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran, Firehed
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6800
Summary:
Ref T3186.
- Every linter builds a WorkingCopyIdentity in the same way, with no specialized data. Don't do that.
- Linters get passed a goofy hardcoded ".php" path. Don't do that.
- Linters generally run on an imaginary path, which might not work. Just give them a real path by building a tiny working copy in `/tmp`.
- Fix a TODO now that we have better typechecking.
Test Plan: `arc unit --everything`, intentionally broke a test to make sure that still works.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6798
Summary:
Ref T2039. That task has a bunch of discussion, but basically we do a poor job of serving the midrange of lint configuration right now.
If you have something simple, the default linters work.
If you have something complex, building your own engine lets you do whatever you want.
But many users want something in between, which isn't really well accommodated. The idea is to let you write a `.arclint` file, which looks something like this:
{
"linters" : {
"css" : {
"type" : "csslint",
"include" : "(\.css$)",
"exclude" : "(^externals/)",
"bin" : "/usr/local/bin/csslint"
},
"js" : {
"type" : "jshint",
"include" : "(\.js$)",
"exclude" : "(^externals/)",
"bin" : "support/bin/jshint",
"interpreter" : "/usr/local/bin/node"
}
}
}
...which will provide a bunch of common options around lint severity, interpreter and binary locaitons, included and excluded files, etc.
This implements some basics, and very rough support in the Filename linter.
Test Plan:
Generated a `.arclint` file and saw it apply filename lint correctly. Used `debug` mode and tried invalid regexps.
{
"debug" : true,
"linters" : {
"filename" : {
"type" : "filename",
"exclude" : ["@^externals/@"]
}
}
}
Next steps include:
- Provide an external linter archetype (T3186) and expose a common set of configuration here ("bin", "interpreter", "flags", "severity").
- Provide a `.arcunit` file which works similarly (it can probably be simpler).
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D6797
Summary:
I suspect that when people use 'algorithmical', they mean it as an adjective and not an adverb.
'algorithmic' = adjective
'algorithmically' = adverb
Test Plan: Add the word 'algorithmical' to a file. Run `arc lint` on the file. See suggestion to correct it to 'algorithmic'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6373
Summary: If one wishes to implement a linter which finds unused resources or variables the current scheme does not work as the lint engine filters all changes from lines which were not introduced in a diff. To solve that, I've added an "always show" configuration for a lint message allowing creation of such linters.
Test Plan:
1. Created a custom linter for finding unused Android resources in a project
2. Ran arc lint with linter added and received warnings as expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6119
Summary:
Piping data around on Windows doesn't work well when it contains zany characters like "null" and "newline". Fixes T3266.
Instead of piping data into `git apply`, write to a temporary file.
Test Plan:
Ran `arc patch`, got good results.
>>> [17] <exec> $ git apply --index --reject -- '/private/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/7z9iea6srikoo0sc/4266-ZEyvz9'
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T3266
Differential Revision: https://secure.phabricator.com/D6070
Summary: Also warn against functions not available on Windows at all.
Test Plan: Compared old and new file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5975
Summary: Detect and fix unconventional spellings of `true`, `false`, `null` and `array` (these are the only keywords I've seen spelled unconventionally in the wild).
Test Plan: Unit tests.
Reviewers: DurhamGoode, btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2985
Differential Revision: https://secure.phabricator.com/D5686
Summary:
I'm guessing this was refactored somewhere down the line and it meant
that Python and Perl files were no longer considered text files.
I'm imagining the old regex was: p(hp|y|l). Therefore I blame CSS.
Test Plan:
Perform an arc lint on a Python or Perl file that has trailing whitespace
on a line. It should prompt you for an autocorrecting lint.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5637
Summary:
On systems with an ancient version of python, the pep8 linter won't run.
Instead of blowing up in the user's face, we should display a nice error
message.
Test Plan:
Put /usr/bin (where the ancient version of python is) at the beginning of
my path and tried to lint some python. I got a nice error instead of a
stack trace.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5356
Summary:
Added a lint rule that warns about reusing iterator reference
variables.
Test Plan:
- Add a file with examples found in with https://secure.phabricator.com/T2536
- Did not make a unit test yet
Reviewers: vrana, bill, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2536
Differential Revision: https://secure.phabricator.com/D5179
Summary:
People constantly forget to bump the linter version and I don't see a way how to stop it.
This may bump the version even if it wouldn't be required but let's rather undercache than overcache.
Test Plan: `var_dump($version)`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5129
Test Plan: Threw in `didRunLinters()` of one linter, still saw the result of other linters and "Some linters failed" at the end.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5124
Summary:
I want to run lint on background and I'm interested only in side effect of caching (and maybe exit status).
This is better than discarding stdout later because we don't do unnecessary work and error conditions are still printed.
Test Plan:
$ arc lint --output none # with error
$ echo $?
$ arc lint --output none # with no lintable paths
$ arc lint --output none # witout errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5106
Summary:
The perf fix actually catches some real problems.
I didn't find anything in libphutil, Arcanist and Phabricator though.
Also bump version.
Also allow configuring the hook.
Test Plan:
Added a test, saw it fail with the old code.
Repeat for hook.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5097
Summary: At least on my sample file.
Test Plan: Saw time 0.073 s instead of 12.606 s.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5086
Summary: This number seems more interesting and it includes time for resolving futures which is the main part of some linters.
Test Plan:
$ arc --trace lint
Reviewers: fdeliege, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5075
Summary:
The message is too defensive.
Test Plan: Tested in the fork.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin, s.o.butler
Differential Revision: https://secure.phabricator.com/D5043
Summary: This is a little bit tricky - if both XHPAST and PhutilXHPAST linters lint the same path then they get the same future wrapped in two different Future iterators.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5015
Summary: Nobody needs it because `raiseLintAtLine()` returns the message.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4870
Summary:
Make all the broad-spectrum text linters use the new binary check.
I didn't touch `ComprehensiveLintEngine` because it's a nest of vipers and no one has complained; see T2039.
Test Plan: Ran `arc lint` on text files and binaries in a libphutil project (arcanist).
Reviewers: lisp
Reviewed By: lisp
CC: aran
Differential Revision: https://secure.phabricator.com/D5040
Summary:
Add linting capability for detecting files which contain
syntax introduced by unresolved merge conflicts. The detection is
file-type-agnostic (the only requirement is that the file is a text
file).
Test Plan:
Tested in three ways.
The first way is to add all three forms of syntax to a file to
indicate a merge conflict. HPHP will pick this up as a syntax error
before this linter reaches it.
The second way is to add the syntax in a comment. In that case, this
linter will show three warnings. For example:
$ arc lint ArcanistMergeConflictLinter.php
>>> Lint for arcanist/src/lint/linter/ArcanistMergeConflictLinter.php:
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
20
21 foreach ($lines as $lineno => $line) {
22 /*
>>> 23 >>>>>>>
24
25 =======
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
22 /*
23 >>>>>>>
24
>>> 25 =======
26
27 <<<<<<<
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
24
25 =======
26
>>> 27 <<<<<<<
28
29 */
The last test was to test on various different file types, including
JavaScript, PHP, an animated GIF, a PNG, and a Bash file to make sure
the file type detection worked. Each of the aforementioned tests
passed.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2547
Differential Revision: https://secure.phabricator.com/D4966
Summary:
Following @epriestley suggestion to use PhutilServiceProfiler to log lint as a service call
Test Plan: arc lint
Reviewers: vrana
CC: phunt, aran, epriestley
Differential Revision: https://secure.phabricator.com/D4820
Summary: This is technically documented, but not currently enforced and we aren't consistent about it in the codebase.
Test Plan: See D5002.
Reviewers: chad, vrana
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5003
Summary: D4963 for other linters.
Test Plan: Saw time 0.001 instead of 0.113 in spellcheck linter.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4965
Summary:
We always generate all messages and then filter them out based on minimum severity.
It's lots of useless work, especially in commit hook mode where we are interested only in errors.
Test Plan:
$ arc lint --cache 0 --severity error ArcanistXHPASTLinter.php
0.406 s before, 0.074 after
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4963
Summary:
We save repository version to lint cache but ignore it when reading the cache.
Fix it.
Test Plan: Made an error for linter with repo granularity, deleted the error from the cache. Relinted, didn't see the error. Changed another file and relinted, saw the error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4841
