fix bugs and documentation for ArcanistPyLintLinter

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

src/lint/linter/pylint/ArcanistPyLintLinter.php

@@ -23,34 +23,41 @@
  *
  * You should be able to install pylint with ##sudo easy_install pylint##. If
  * your system is unusual, you can manually specify the location of pylint and
- * its dependencies by configuring these keys in your .arcrc:
+ * its dependencies by configuring these keys in your .arcconfig:
  *
  *   lint.pylint.prefix
  *   lint.pylint.logilab_astng.prefix
  *   lint.pylint.logilab_common.prefix
  *
- * You can specify additional command-line options to pass to PyLint by setting
- * this key:
+ * You can specify additional command-line options to pass to PyLint by
+ * setting ##lint.pylint.options##.
+
  *
- *   lint.pylint.options
- *
- * Now, configure which warnings and errors you want PyLint to raise by setting
- * these keys:
+ * Specify which PyLint messages map to which Arcanist messages by defining
+ * the following regular expressions:
  *
  *   lint.pylint.codes.error
  *   lint.pylint.codes.warning
  *   lint.pylint.codes.advice
  *
- * Set these to regular expressions -- for instance, if you want to raise all
- * PyLint errors as Arcanist errors, set this for ##lint.pylint.codes.error##:
+ * The regexps are run in that order; the first to match determines which
+ * Arcanist severity applies, if any. For example, to capture all PyLint
+ * "E...." errors as Arcanist errors, set ##lint.pylint.codes.error## to:
  *
  *    ^E.*
  *
- * You can also match more granular errors:
+ * You can also match more granularly:
  *
  *    ^E(0001|0002)$
  *
- * You can also provide a list of regular expressions.
+ * According to ##man pylint##, there are 5 kind of messages:
+ *
+ *   (C) convention, for programming standard violation
+ *   (R) refactor, for bad code smell
+ *   (W) warning, for python specific problems
+ *   (E) error, for probable bugs in the code
+ *   (F) fatal, if an error occurred which prevented pylint from
+ *       doing further processing.
  *
  * @group linter
  */
@@ -60,11 +67,6 @@ class ArcanistPyLintLinter extends ArcanistLinter {
 
     $working_copy = $this->getEngine()->getWorkingCopy();
 
-    // The config file defines how PyLint message codes translate to
-    // arcanist severities.  The config options provide regex's to
-    // match against the message codes generated by PyLint.  Severity's
-    // are matched in the order of errors, warnings, then advice.
-    // The first severity that matches, in that order, is returned.
     $error_regexp   = $working_copy->getConfig('lint.pylint.codes.error');
     $warning_regexp = $working_copy->getConfig('lint.pylint.codes.warning');
     $advice_regexp  = $working_copy->getConfig('lint.pylint.codes.advice');
@@ -150,16 +152,15 @@ class ArcanistPyLintLinter extends ArcanistLinter {
   }
 
   private function getPyLintOptions() {
-    // Options to pass the PyLint
-    // - '-rn': don't print lint report/summary at end
-    // - '-iy': show message codes for lint warnings/errors
+    // '-rn': don't print lint report/summary at end
+    // '-iy': show message codes for lint warnings/errors
     $options = array('-rn',  '-iy');
 
     // Add any options defined in the config file for PyLint
     $working_copy = $this->getEngine()->getWorkingCopy();
     $config_options = $working_copy->getConfig('lint.pylint.options');
     if ($config_options !== null) {
-      $options += $config_options;
+      $options = array_merge($options, $config_options);
     }
 
     return implode(" ", $options);
@@ -193,16 +194,16 @@ class ArcanistPyLintLinter extends ArcanistLinter {
             "{$pylint_bin} {$options} {$path_on_disk}",
           $python_path);
     } catch (CommandException $e) {
-      // PyLint will return a non-zero exit code if warnings/errors are found.
-      // Therefore we detect command failure by checking that the stderr is
-      // some non-expected value.
-      if ($e->getStderr() !== "No config file found, ".
-                              "using default configuration\n") {
+      if ($e->getError() == 32) {
+        // According to ##man pylint## the exit status of 32 means there was a
+        // usage error. That's bad, so actually exit abnormally.
         throw $e;
-      }
-
+      } else {
+        // The other non-zero exit codes mean there were messages issued,
+        // which is expected, so don't exit.
         $stdout = $e->getStdout();
       }
+    }
 
     $lines = explode("\n", $stdout);
     $messages = array();