Page MenuHomePhabricator

Don't bundle PEP8
ClosedPublic

Authored by joshuaspence on Jun 9 2014, 3:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 21, 5:23 PM
Unknown Object (File)
Fri, Dec 13, 9:38 PM
Unknown Object (File)
Fri, Dec 13, 9:38 PM
Unknown Object (File)
Fri, Dec 13, 12:59 AM
Unknown Object (File)
Thu, Dec 12, 6:10 PM
Unknown Object (File)
Thu, Dec 12, 8:54 AM
Unknown Object (File)
Sun, Dec 8, 5:58 PM
Unknown Object (File)
Wed, Dec 4, 10:06 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
Branch
rm-pep8
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 930
Build 930: [Placeholder Plan] Wait for 30 Seconds

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.