Improve lint integration documentation

Summary: Explain what lint is, how to configure it, what we ship with, etc. Also wax philosophical.

Test Plan: Generated docs, read "carefully".

Reviewers: csilvers, btrahan, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D2628

src/docs/userguide/arcanist_extending_lint.diviner

@@ -9,7 +9,8 @@ Arcanist ships with a number of linters which you may want to reuse in whole
 or in part in other projects. This document explains how to customize existing
 linters for use in new engines.
 
-First, you should set up a custom engine by following the steps in
+First, you should set up an engine by following the instructions in
+@{article:Arcanist User Guide: Lint} and possibly
 @{article:Arcanist User Guide: Customizing Lint, Unit Tests and Workflows}.
 Then, follow this guide to customize linters.
 
@@ -17,7 +18,7 @@ Then, follow this guide to customize linters.
 
 You should customize linters by configuring or composing them, not by extending
 them -- their implementations are not necessarily stable. If a linter's
-configuration options aren't flexible enough to meet your needs, a patch
+configuration options aren't flexible enough to meet your needs, sending a patch
 which improves its configurability is better than one that makes it
 nonfinal.
 

src/docs/userguide/arcanist_lint.diviner

@@ -0,0 +1,190 @@
+@title Arcanist User Guide: Lint
+@group userguide
+
+Guide to lint, linters, and linter configuration.
+
+= Overview =
+
+"Lint" refers to a general class of programming tools which analyze source code
+and raise warnings and errors about it. For example, a linter might raise
+warnings about syntax errors, uses of undeclared variables, calls to deprecated
+functions, spacing and formatting conventions, misuse of scope, implicit
+fallthrough in switch statements, missing license headers, use of dangerous
+language features, or a variety of other issues.
+
+Integrating lint into your development pipeline has two major benefits:
+
+  - you can detect and prevent a large class of programming errors; and
+  - you can simplify code review by addressing many mechanical and formatting
+    problems automatically.
+
+When arc is integrated with a lint toolkit, it enables the `arc lint` command
+and runs lint on changes during `arc diff`. The user is prompted to fix errors
+and warnings before sending their code for review, and lint issues which are
+not fixed are visible during review.
+
+There are many lint and static analysis tools available for a wide variety of
+languages. Arcanist ships with bindings for many popular tools, and you can
+write new bindings fairly easily if you have custom tools.
+
+= Available Linters =
+
+Arcanist ships with bindings for these linters:
+
+  - [[http://www.jshint.com/ | JSHint]], a Javascript linter based on JSHint.
+    See @{class@arcanist:ArcanistJSHintLinter}.
+  - [[http://pypi.python.org/pypi/pep8 | PEP8]], a Python linter.
+    See @{class@arcanist:ArcanistPEP8Linter}.
+  - [[http://pypi.python.org/pypi/pyflakes | Pyflakes]], another Python linter.
+    See @{class@arcanist:ArcanistPyFlakesLinter}.
+  - [[http://pypi.python.org/pypi/pylint | Pylint]], yet another Python linter.
+    See @{class@arcanist:ArcanistPyLintLinter}.
+  - [[http://pear.php.net/package/PHP_CodeSniffer | PHP CodeSniffer]], a
+    PHP linter. See @{class@arcanist:ArcanistPhpcsLinter}.
+
+Arcanist also ships with generic bindings which can be configured to parse the
+output of a broad range of lint programs:
+
+  - @{class@arcanist:ArcanistScriptAndRegexLinter}, which runs a script and
+    parses its output with a regular expression.
+  - @{class@arcanist:ArcanistConduitLinter}, which invokes a linter over
+    Conduit and can allow you to build client/server linters.
+
+Additionally, Arcanist ships with some general purpose linters:
+
+  - @{class@arcanist:ArcanistTextLinter}, which enforces basic things like
+    trailing whitespace, DOS newlines, file encoding, line widths, terminal
+    newlines, and tab literals.
+  - @{class@arcanist:ArcanistSpellingLinter}, which can detect common spelling
+    mistakes.
+  - @{class@arcanist:ArcanistFilenameLinter}, which can enforce generally
+    sensible rules about not giving files nonsense names.
+  - @{class@arcanist:ArcanistLicenseLinter.php}, which can make sure license
+    headers are applied to all source files.
+  - @{class@arcanist:ArcanistNoLintLinter}, which can disable lint for files
+    marked unlintable.
+  - @{class@arcanist:ArcanistGeneratedLinter}, which can disable lint for
+    generated files.
+
+Finally, Arcanist has special-purpose linters:
+
+  - @{class@arcanist:ArcanistXHPASTLinter}, the PHP linter used by Phabricator
+    itself. This linter is powerful, but somewhat rigid (it enforces phutil
+    rules and isn't very configurable for other rulesets).
+  - @{class@arcanist:ArcanistPhutilLibraryLinter}, which enforces phutil library
+    layout rules.
+
+You can add support for new linters in three ways:
+
+  - write new bindings and contribute them to the upstream;
+  - write new bindings and install them alongside Arcanist; or
+  - use a generic binding like @{class@arcanist:ArcanistScriptAndRegexLinter}
+    and drive the integration through configuration.
+
+= Configuring Lint =
+
+Arcanist's lint integration involves two major components: linters and lint
+engines.
+
+Linters themselves are programs which detect problems in a source file. Usually
+a linter is an external script, which Arcanist runs and passes a path to, like
+`jshint` or `pep8.py`. The script emits some messages, and Arcanist parses
+the output into structured errors. A piece of glue code (like
+@{class@arcanist:ArcanistJSHintLinter} or
+@{class@arcanist:ArcanistPEP8Linter}) handles calling the external script and
+interpreting its output.
+
+Lint engines coordinate linters, and decide which linters should run on which
+files. For instance, you might want to run `jshint` on all your `.js` files,
+and `pep8.py` on all your `.py` files. And you might not want to lint anything
+in `externals/` or `third-party/`, and maybe there are other files which you
+want to exclude or apply special rules for.
+
+To configure arc for lint, you specify the name of a lint engine, and possibly
+provide some additional configuration. To name a lint engine, set `lint.engine`
+in your `.arcconfig` to the name of a class which extends
+@{class@arcanist:ArcanistLintEngine}. For more information on `.arcconfig`, see
+@{article:Arcanist User Guide: Configuring a New Project}.
+
+You can also set a default lint engine by setting `lint.engine` in your global
+user config with `arc set-config lint.engine`, or specify one explicitly with
+`arc lint --engine <engine>`.
+
+The available engines are:
+
+  - @{class@arcanist:ComprehensiveLintEngine}, which runs a wide array of
+    linters on many types of files. This is probably of limited use in any real
+    project because it is overbroad, but is a good starting point for getting
+    lint doing things.
+  - @{class@arcanist:ArcanistSingleLintEngine}, which runs a single linter on
+    every file unconditionally. This can be used with a glue linter like
+    @{class@arcanist:ArcanistScriptAndRegexLinter} to put engine logic in an
+    external script.
+  - A custom engine you write. For most projects, lint rules are sufficiently
+    specialized that this is the best option. For instructions on writing a
+    custom lint engine, see
+    @{article:Arcanist User Guide: Customizing Lint, Unit Tests and Workflows}
+    and @{class@arcanist:ExampleLintEngine}.
+
+= Using Lint to Improve Code Review =
+
+Code review is most valuable when it's about the big ideas in a change. It is
+substantially less valuable when it devolves into nitpicking over style,
+formatting, and naming conventions.
+
+The best response to receiving a review request full of style problems is
+probably to reject it immediately, point the author at your coding convention
+documentation, and ask them to fix it before sending it for review. But even
+this is a pretty negative experience for both parties, and less experienced
+reviewers sometimes go through the whole review and point out every problem
+individually.
+
+Lint can greatly reduce the negativity of this whole experience (and the amount
+of time wasted arguing about these things) by enforcing style and formatting
+rules automatically. Arcanist supports linters that not only raise warnings
+about these problems, but provide patches and fix the problems for the author --
+before the code goes to review.
+
+Good linter integration means that code is pretty much mechanically correct by
+the time any reviewer sees it, provides clear rules about style which are
+especially helpful to new authors, and has the overall effect of pushing
+discussion away from stylistic nitpicks and toward useful examination of large
+ideas.
+
+It can also provide a straightforward solution to arguments about style:
+
+  - If a rule is important enough that it should be enforced, the proponent must
+    add it to lint so it is automatically detected or fixed in the future and
+    no one has to argue about it ever again.
+  - If it's not important enough for them to do the legwork to add it to lint,
+    they have to stop complaining about it.
+
+This may or may not be an appropriate methodology to adopt at your organization,
+but it generally puts the incentives in the right places.
+
+= Philosophy of Lint =
+
+Some general thoughts on how to develop lint effectively, based on building
+lint tools at Facebook:
+
+  - Don't write regex-based linters to enforce language rules. Use a real parser
+    or AST-based tool. This is not a domain you can get right at any nontrivial
+    complexity with raw regexes. That is not a challenge. Just don't do this.
+  - False positives are pretty bad and should be avoided. You should aim to
+    implement only rules that have very few false positives, and provide ways to
+    mark false positives as OK. If running lint always raises 30 warnings about
+    irrelevant nonsense, it greatly devalues the tool.
+  - Move toward autocorrect rules. Most linters do not automatically correct
+    the problems they detect, but Arcanist supports this and it's quite
+    valuable to have the linter not only say "the convention is to put a space
+    after comma in a function call" but to fix it for you.
+
+= Next Steps =
+
+Continue by:
+
+  - integrating and customizing built-in linters and lint bindings with
+    @{article:Arcanist User Guide: Customizing Existing Linters}; or
+  - learning how to add new linters and lint engines with
+    @{article:Arcanist User Guide: Customizing Lint, Unit Tests and Workflows}.
+

src/docs/userguide/arcanist_lint_unit.diviner

@@ -5,7 +5,7 @@ Explains how to build new classes to control how Arcanist behaves.
 
 = Overview =
 
-Arcanist has some basic configuration options available in the ##.arcconfig##
+Arcanist has some basic configuration options available in the `.arcconfig`
 file (see @{article:Arcanist User Guide: Configuring a New Project}), but it
 can't handle everything. If you want to customize Arcanist at a deeper level,
 you need to build new classes. For instance:
@@ -19,16 +19,16 @@ you need to build new classes. For instance:
     @{class@arcanist:ArcanistConfiguration}.
 
 Arcanist works through a sort of dependency-injection approach. For example,
-Arcanist does not run lint rules by default, but you can set **lint_engine**
+Arcanist does not run lint rules by default, but you can set `lint.engine`
-in your ##.arcconfig## to the name of a class which extends
+in your `.arcconfig` to the name of a class which extends
 @{class@arcanist:ArcanistLintEngine}. When running from inside your project,
 Arcanist will load this class and call methods on it in order to run lint. To
 make this work, you need to do three things:
 
   - actually write the class;
   - add the library where the class exists to your ##.arcconfig##;
-  - add the class name to your ##.arcconfig## as the **lint_engine**,
+  - add the class name to your ##.arcconfig## as the **lint.engine**,
-    **unit_engine**, or **arcanist_configuration**.
+    **unit.engine**, or **arcanist_configuration**.
 
 = Create a libphutil Library =
 
@@ -38,12 +38,12 @@ then make the library loadable by adding it to your ##.arcconfig## like this:
 
   {
     // ...
-    "phutil_libraries" : {
+    "phutil_libraries" : [
       // ...
-      "library-a" : "/path/to/my/library", // Absolute path
+      "/path/to/my/library", // Absolute path
-      "library-b" : "support/arcanist",    // Relative path in this project
+      "support/arcanist",    // Relative path in this project
       // ...
-    }
+    ]
     // ...
   }
 
@@ -67,7 +67,7 @@ the appropriate configuration value.
 
   {
     // ...
-    "lint_engine" : "CustomArcanistLintEngine",
+    "lint.engine" : "CustomArcanistLintEngine",
     // ...
   }
 

src/docs/userguide/arcanist_new_project.diviner

@@ -76,11 +76,10 @@ For an exhaustive list of available options, see below.
 
 Other options include:
 
-  - **lint_engine**: the name of a subclass of
+  - **lint.engine**: the name of a subclass of
     @{class@arcanist:ArcanistLintEngine}, which should be used to apply lint
-    rules to this project. See @{article:Arcanist User Guide: Customizing Lint,
+    rules to this project. See @{article:Arcanist User Guide: Lint}.
-    Unit Tests and Workflows}.
+  - **unit.engine**: the name of a subclass of
-  - **unit_engine**: the name of a subclass of
     @{class@arcanist:ArcanistBaseUnitTestEngine}, which should be used to apply
     unit test rules to this project. See
     @{article:Arcanist User Guide: Customizing Lint, Unit Tests and Workflows}.