Page MenuHomePhabricator
Differential D9109

Modernize `ArcanistPylintLinter`
ClosedPublic
Actions

Authored by joshuaspence on May 13 2014, 10:21 PM.
Tags
None
Referenced Files
F14080221: D9109.id27141.diff
Fri, Nov 22, 12:10 PM
F14080220: D9109.id27101.diff
Fri, Nov 22, 12:10 PM
F14080219: D9109.id27100.diff
Fri, Nov 22, 12:10 PM
F14080216: D9109.id21643.diff
Fri, Nov 22, 12:10 PM
F14080214: D9109.id28200.diff
Fri, Nov 22, 12:09 PM
F14080212: D9109.id30870.diff
Fri, Nov 22, 12:09 PM
F14080207: D9109.id27600.diff
Fri, Nov 22, 12:08 PM
F14080206: D9109.id.diff
Fri, Nov 22, 12:08 PM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T5222: Exception pylint is an invalid linter
Restricted Maniphest Task
Commits
rARCa6a26bb3a30a: Modernize `ArcanistPylintLinter`
Summary

Ref T2039. Convert the ArcanistPylintLinter to an ArcanistExternalLinter and make it compatible with .arclint. In doing so, it was necessary to drop support of older versions of pylint by setting the minimum version to be 1.0.0. I think that dropping support for older versions is reasonable because version 1.0.0 was released ~18 months ago. In the case than an incompatible version is detected, an ArcanistMissingLinterException is thrown.

One caveat here is that support for lint.pylint.codes.(error|warning|advice) is dropped. Any installs that are relying on this configuration will need to migrate to using the .arclint file for specifying linter severity. We could potentially continue to support this deprecated configuration, but I'm not sure if this is worthwhile.

Test Plan

Wrote and executed unit tests with arc unit. I ran the unit tests on all pylint releases after (and including) 1.0.0. Specifically, the tests were run on v1.0.0, v1.1.0, v1.2.0, v1.3.0 and v1.4.0.

Diff Detail

Repository
rARC Arcanist
Branch
pylint
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/lint/linter/ArcanistPyLintLinter.php:32XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 444
Build 444: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 21643.May 13 2014, 10:21 PM
joshuaspence retitled this revision from to Modernize `ArcanistPylintLinter`..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence added a task: Restricted Maniphest Task.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 13 2014, 10:21 PM
Herald added subscribers: Korvin, epriestley. · View Herald Transcript
Harbormaster completed remote builds in B444: Diff 21643.May 13 2014, 10:22 PM
joshuaspence added a comment.May 13 2014, 10:24 PM

I probably should bring the lint.pylint.code.error stuff over as well... Although this is much better suited to being customized with the existing severity stuff.

epriestley requested changes to this revision.May 17 2014, 2:14 AM
epriestley edited edge metadata.

I think we're going to catch a lot of heat for this one no matter what, but being aggressive about it is reasonable since the old one was so insane that we can't possibly retain all the old behaviors.

I would like to retain the 1.0 vs prior-to-1.0 stuff, though, or wait for the minimum version stuff to be user-facing to ship this. Does the new version work on both sides of the version boundary?

src/lint/linter/ArcanistPyLintLinter.php
34–35

Use getDeprecatedConfig()?

38

I think this "/bin/" isn't present on the left?

82

Probably a double escape now?

107

Oh, here's the "/bin/". This is, uh, odd.

162–173

This logic was added recently and we should retain it, unless you've implicitly fixed it by choosing flag more intelligently.

233

We might need to add env to .arclint for "random junk to add to the environment", but let's wait until someone complains.

This revision now requires changes to proceed.May 17 2014, 2:14 AM
joshuaspence added a comment.May 17 2014, 2:45 AM
In D9109#8, @epriestley wrote:

I think we're going to catch a lot of heat for this one no matter what, but being aggressive about it is reasonable since the old one was so insane that we can't possibly retain all the old behaviors.

I would like to retain the 1.0 vs prior-to-1.0 stuff, though, or wait for the minimum version stuff to be user-facing to ship this. Does the new version work on both sides of the version boundary?

Let me test this out on a couple of different versions.

src/lint/linter/ArcanistPyLintLinter.php
34–35

Yep, you're right. I probably started working on this before getDeprecatedConfig existed.

82

Yeah possibly, I want to to double check this but I think that you are right.

107

Yeah, I have no idea what this is for.

joshuaspence added a comment.May 18 2014, 2:30 AM
In D9109#8, @epriestley wrote:

I think we're going to catch a lot of heat for this one no matter what, but being aggressive about it is reasonable since the old one was so insane that we can't possibly retain all the old behaviors.

I would like to retain the 1.0 vs prior-to-1.0 stuff, though, or wait for the minimum version stuff to be user-facing to ship this. Does the new version work on both sides of the version boundary?

I think we should just wait for D9161 and then drop support for pylint older than v1.0.0.

joshuaspence added a parent revision: D9161: Allow version dependencies to be specified for external linters.May 18 2014, 2:35 AM
joshuaspence updated this revision to Diff 21793.May 18 2014, 2:35 AM
joshuaspence edited edge metadata.

Made a few minor changes, mainly to address @epriestley's comments.

Harbormaster completed remote builds in B541: Diff 21793.May 18 2014, 2:36 AM
joshuaspence planned changes to this revision.May 18 2014, 2:38 AM

I will rebase this after D9161 lands and specify that the minimum required version of pylint is 1.0.0.

joshuaspence retitled this revision from Modernize `ArcanistPylintLinter`. to Modernize `ArcanistPylintLinter`.Sep 22 2014, 10:10 AM
joshuaspence edited edge metadata.
joshuaspence updated this revision to Diff 27100.Jan 8 2015, 12:07 PM

Rebase

joshuaspence updated this revision to Diff 27101.Jan 8 2015, 12:10 PM

oops

joshuaspence updated this revision to Diff 27141.Jan 9 2015, 12:07 PM

Minor tidying

joshuaspence updated this object.Jan 9 2015, 12:10 PM
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence removed a parent revision: D9161: Allow version dependencies to be specified for external linters.Jan 20 2015, 10:26 AM
joshuaspence updated this revision to Diff 27600.Jan 23 2015, 11:45 AM

Rebase, fix method visibility

joshuaspence edited the test plan for this revision. (Show Details)Jan 26 2015, 4:20 AM
joshuaspence updated this revision to Diff 28200.Feb 9 2015, 7:05 AM

Move --rcfile to getMandatoryFlags()

joshuaspence updated this object.Feb 10 2015, 12:20 PM
joshuaspence updated this object.
joshuaspence updated this object.Feb 10 2015, 12:24 PM
epriestley accepted this revision.May 13 2015, 1:59 PM
epriestley edited edge metadata.

whatcouldgowrong

src/lint/linter/ArcanistPyLintLinter.php
146

Setting this explicitly is probably wrong (it won't respect .arclint configuration)?

I think you need to implement getDefaultCodeSeverity() or whatever insetad.

src/lint/linter/__tests__/pylint/empty.lint-test
2 ↗(On Diff #28200)

Maybe beef this up very slightly?

This revision is now accepted and ready to land.May 13 2015, 1:59 PM
joshuaspence marked 2 inline comments as done.May 14 2015, 7:59 AM
joshuaspence added inline comments.
src/lint/linter/__tests__/pylint/empty.lint-test
2 ↗(On Diff #28200)

I'll just remove this... we already have an existing test case which should suffice.

joshuaspence updated this revision to Diff 30870.May 14 2015, 8:01 AM
joshuaspence marked an inline comment as done.
joshuaspence edited edge metadata.

Minor tidying

Closed by commit rARCa6a26bb3a30a: Modernize `ArcanistPylintLinter` (authored by joshuaspence, committed by joshuaspence). · Explain WhyMay 14 2015, 8:08 AM
This revision was automatically updated to reflect the committed changes.
joshuaspence added a task: T5222: Exception pylint is an invalid linter.May 28 2015, 9:12 PM
joshuaspence mentioned this in T5222: Exception pylint is an invalid linter.

Revision Contents
Changeset List

PathSize
src/
4 lines
lint/
linter/
332 lines
__tests__/
12 lines
pylint/
7 lines

Diff 21643

View Options

src/__phutil_library_map__.php

Loading...
View Options

src/lint/linter/ArcanistPyLintLinter.php

Loading...
View Options

src/lint/linter/__tests__/ArcanistPyLintLinterTestCase.php

Loading...
View Options

src/lint/linter/__tests__/pylint/pylint.lint-test

Loading...
Phabricator · Built by Phacility