Page MenuHomePhabricator

Don't bundle PEP8
ClosedPublic

Authored by joshuaspence on Jun 9 2014, 3:28 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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

jshint is the default?

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

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.