Page MenuHomePhabricator

Don't bundle PEP8
ClosedPublic

Authored by joshuaspence on Jun 9 2014, 3:28 PM.
Tags
None
Referenced Files
F13329737: D9430.diff
Mon, Jun 17, 1:26 AM
F13300776: D9430.id31093.diff
Fri, Jun 7, 9:38 AM
F13298634: D9430.diff
Fri, Jun 7, 7:03 AM
F13283061: D9430.diff
Sun, Jun 2, 12:46 PM
F13267736: D9430.diff
Wed, May 29, 4:07 AM
F13266976: D9430.id.diff
Tue, May 28, 10:59 PM
F13253716: D9430.diff
Sat, May 25, 2:53 AM
F13238613: D9430.id31147.diff
Tue, May 21, 8:29 PM

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.
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 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
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?

epriestley edited edge metadata.

jshint default is definitely wrong.

This revision now requires changes to proceed.May 19 2015, 1:27 PM
joshuaspence added inline comments.
src/lint/linter/ArcanistPEP8Linter.php
43–44

This should already happen in ArcanistExternalLinter.

joshuaspence edited edge metadata.
joshuaspence marked an inline comment as done.

Fix bad copy-paste

epriestley edited edge metadata.
This revision is now accepted and ready to land.May 19 2015, 2:15 PM
This revision was automatically updated to reflect the committed changes.