From 06334a69b4dd1dfab966710ccd988dce410fc190 Mon Sep 17 00:00:00 2001
From: David Cramer <dcramer@gmail.com>
Date: Tue, 17 Jan 2012 12:38:54 -0800
Subject: [PATCH] Improve PEP8 Linter

Summary:
This cleans up the PEP8 linter to bring it inline
with the new JSHint Linter's level of quality.

It adds a getPEP8Path method which gives the ability
for users to override the pep8 binary with two new
options in .arcconfig:

* lint.pep8.prefix
* lint.pep8.bin
* lint.pep8.options

Test Plan:
Adjust your engine to use the 'ArcanistPEP8Linter' and run
arc lint against Python files.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1440
---
 src/__phutil_library_map__.php              |  4 +-
 src/lint/linter/pep8/ArcanistPEP8Linter.php | 68 ++++++++++++++++++---
 src/lint/linter/pep8/__init__.php           |  3 +
 3 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9e24b9c5..2279b6cf 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -99,7 +99,7 @@ phutil_register_library_map(array(
     'ArcanistXHPASTLinter' => 'lint/linter/xhpast',
     'ArcanistXHPASTLinterTestCase' => 'lint/linter/xhpast/__tests__',
     'BranchInfo' => 'branch',
-    'ComprehensiveEngine' => 'lint/engine/comprehensive',
+    'ComprehensiveLintEngine' => 'lint/engine/comprehensive',
     'ExampleLintEngine' => 'lint/engine/example',
     'PhutilLintEngine' => 'lint/engine/phutil',
     'PhutilModuleRequirements' => 'parser/phutilmodule',
@@ -166,7 +166,7 @@ phutil_register_library_map(array(
     'ArcanistUserAbortException' => 'ArcanistUsageException',
     'ArcanistXHPASTLinter' => 'ArcanistLinter',
     'ArcanistXHPASTLinterTestCase' => 'ArcanistLinterTestCase',
-    'ComprehensiveEngine' => 'ArcanistLintEngine',
+    'ComprehensiveLintEngine' => 'ArcanistLintEngine',
     'ExampleLintEngine' => 'ArcanistLintEngine',
     'PhutilLintEngine' => 'ArcanistLintEngine',
     'PhutilUnitTestEngine' => 'ArcanistBaseUnitTestEngine',
diff --git a/src/lint/linter/pep8/ArcanistPEP8Linter.php b/src/lint/linter/pep8/ArcanistPEP8Linter.php
index 92343255..d05bc9b6 100644
--- a/src/lint/linter/pep8/ArcanistPEP8Linter.php
+++ b/src/lint/linter/pep8/ArcanistPEP8Linter.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -36,25 +36,73 @@ class ArcanistPEP8Linter extends ArcanistLinter {
   }
 
   public function getLintNameMap() {
-    return array(
-    );
+    return array();
   }
 
   public function getPEP8Options() {
-    // W293 (blank line contains whitespace) is redundant when used
-    // alongside TXT6, causing pain to python programmers.
-    return '--ignore=W293';
+    $working_copy = $this->getEngine()->getWorkingCopy();
+    $options = $working_copy->getConfig('lint.pep8.options');
+
+    if ($options === null) {
+      // W293 (blank line contains whitespace) is redundant when used
+      // alongside TXT6, causing pain to python programmers.
+      return '--ignore=W293';
+    }
+
+    return $options;
+  }
+
+  public function getPEP8Path() {
+    $working_copy = $this->getEngine()->getWorkingCopy();
+    $prefix = $working_copy->getConfig('lint.pep8.prefix');
+    $bin = $working_copy->getConfig('lint.pep8.bin');
+
+    if ($bin === null && $prefix === null) {
+      $bin = csprintf('/usr/bin/env python2.6 %s',
+               phutil_get_library_root('arcanist').
+               '/../externals/pep8/pep8.py');
+    }
+    else {
+      if ($bin === null) {
+        $bin = 'pep8';
+      }
+
+      if ($prefix !== null) {
+        if (!Filesystem::pathExists($prefix.'/'.$bin)) {
+          throw new ArcanistUsageException(
+            "Unable to find PEP8 binary in a specified directory. Make sure ".
+            "that 'lint.pep8.prefix' and 'lint.pep8.bin' keys are set ".
+            "correctly. If you'd rather use a copy of PEP8 installed ".
+            "globally, you can just remove these keys from your .arcconfig");
+        }
+
+        $bin = csprintf("%s/%s", $prefix, $bin);
+
+        return $bin;
+      }
+
+      // Look for globally installed PEP8
+      list($err) = exec_manual('which %s', $bin);
+      if ($err) {
+        throw new ArcanistUsageException(
+          "PEP8 does not appear to be installed on this system. Install it ".
+          "(e.g., with 'easy_install pep8') or configure ".
+          "'lint.pep8.prefix' in your .arcconfig to point to the directory ".
+          "where it resides.");
+      }
+    }
+
+    return $bin;
   }
 
   public function lintPath($path) {
-    $pep8_bin = phutil_get_library_root('arcanist').
-                  '/../externals/pep8/pep8.py';
-
+    $pep8_bin = $this->getPEP8Path();
     $options = $this->getPEP8Options();
 
     list($rc, $stdout) = exec_manual(
-      "/usr/bin/env python2.6 %s {$options} %s",
+      "%C %C %s",
       $pep8_bin,
+      $options,
       $this->getEngine()->getFilePathOnDisk($path));
 
     $lines = explode("\n", $stdout);
diff --git a/src/lint/linter/pep8/__init__.php b/src/lint/linter/pep8/__init__.php
index 34660432..4dfb4b53 100644
--- a/src/lint/linter/pep8/__init__.php
+++ b/src/lint/linter/pep8/__init__.php
@@ -6,12 +6,15 @@
 
 
 
+phutil_require_module('arcanist', 'exception/usage');
 phutil_require_module('arcanist', 'lint/linter/base');
 phutil_require_module('arcanist', 'lint/message');
 phutil_require_module('arcanist', 'lint/severity');
 
+phutil_require_module('phutil', 'filesystem');
 phutil_require_module('phutil', 'future/exec');
 phutil_require_module('phutil', 'moduleutils');
+phutil_require_module('phutil', 'xsprintf/csprintf');
 
 
 phutil_require_source('ArcanistPEP8Linter.php');