Page MenuHomePhabricator

Modernize `ArcanistPylintLinter`
ClosedPublic

Authored by joshuaspence on May 13 2014, 10:21 PM.
Tags
None
Referenced Files
F12846280: D9109.diff
Fri, Mar 29, 12:52 AM
Unknown Object (File)
Tue, Mar 5, 7:12 AM
Unknown Object (File)
Feb 11 2024, 6:48 AM
Unknown Object (File)
Feb 7 2024, 9:32 AM
Unknown Object (File)
Feb 5 2024, 7:51 AM
Unknown Object (File)
Feb 5 2024, 3:25 AM
Unknown Object (File)
Feb 5 2024, 3:25 AM
Unknown Object (File)
Feb 5 2024, 3:25 AM
Subscribers

Details

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

Event Timeline

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.

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 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
36–42

Use getDeprecatedConfig()?

44

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

97–99

Probably a double escape now?

107

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

165–176

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

232

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
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
36–42

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

97–99

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.

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 edited edge metadata.

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

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 edited the test plan for this revision. (Show Details)

Move --rcfile to getMandatoryFlags()

epriestley edited edge metadata.

whatcouldgowrong

src/lint/linter/ArcanistPyLintLinter.php
155

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 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 marked an inline comment as done.
joshuaspence edited edge metadata.

Minor tidying

This revision was automatically updated to reflect the committed changes.