Don't bundle PEP8
ClosedPublic
Actions

Authored by joshuaspence on Jun 9 2014, 3:28 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rARC5b2571f5ae44: Don't bundle PEP8

Summary

Currently, we bundle a specific version of pep8 with Arcanist. Whilst maybe this made sense historically, as pointed out by @epriestley in D9412#6, there is no longer much point in doing so.

Test Plan

N/A

Diff Detail

Repository

rARC Arcanist

Branch

master

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 6110
Build 6130: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 22514.Jun 9 2014, 3:28 PM

joshuaspence retitled this revision from to Don't bundle `pep8.py`..

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

joshuaspence added a subscriber: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 9 2014, 3:28 PM

Herald added a subscriber: Korvin. · View Herald Transcript

This can be landed whenever.

Harbormaster completed remote builds in B930: Diff 22514.Jun 9 2014, 3:29 PM

joshuaspence retitled this revision from Don't bundle `pep8.py`. to [LATER] Don't bundle `pep8.py`..Jun 11 2014, 1:14 AM

joshuaspence edited edge metadata.

joshuaspence mentioned this in D9474: Don't explicitly use an interpreter for `pep8`.Sep 22 2014, 8:38 AM

joshuaspence retitled this revision from [LATER] Don't bundle `pep8.py`. to [LATER] Don't bundle `pep8.py`.Sep 22 2014, 2:24 PM

joshuaspence retitled this revision from [LATER] Don't bundle `pep8.py` to Don't bundle `pep8.py`.Dec 30 2014, 11:36 PM

FWIW, a colleague of mine was confused by the existing behavior. Specifically, it was unclear why arc lint was looking for python2.6 (presumably, he had python2.7 installed locally). The "fix" was to pip install pep8.

joshuaspence retitled this revision from Don't bundle `pep8.py` to Don't bundle PEP8.Feb 8 2015, 11:19 AM

vrusinov added a subscriber: vrusinov.Mar 23 2015, 6:00 PM

talshiri added a subscriber: talshiri.Mar 23 2015, 6:53 PM

talshiri added inline comments.

src/lint/linter/ArcanistPEP8Linter.php
43–44	Feels like this should not be deleted. Instead, binaryExists() should potentially be used to figure out if pep8 is even installed, and if not raise an exception. This would be more like how ArcanistCSharpLinter works
44	jshint is the default?